<< Back to previous view

[CLJ-1668] ns macro throws NPE if empty reference is specified Created: 02/Mar/15  Updated: 02/Mar/15

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

Type: Defect Priority: Minor
Reporter: Philipp Meier Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, macro, namespace


 Description   

The following invocations of `ns` will all throw a NPE

(ns foo ())
(ns foo [])
(ns foo (:require clojure.core) ())

;; throw


1. Unhandled java.lang.NullPointerException
   (No message)

                      core.clj: 1518  clojure.core/name
                      core.clj: 5330  clojure.core/ns/process-reference
                      core.clj: 2559  clojure.core/map/fn
                  LazySeq.java:   40  clojure.lang.LazySeq/sval
                  LazySeq.java:   49  clojure.lang.LazySeq/seq
                       RT.java:  484  clojure.lang.RT/seq
                      core.clj:  133  clojure.core/seq
                      core.clj:  694  clojure.core/concat/cat/fn
                  LazySeq.java:   40  clojure.lang.LazySeq/sval
                  LazySeq.java:   49  clojure.lang.LazySeq/seq
                     Cons.java:   39  clojure.lang.Cons/next
                       RT.java: 1654  clojure.lang.RT/boundedLength
                      AFn.java:  148  clojure.lang.AFn/applyToHelper
                      Var.java:  700  clojure.lang.Var/applyTo

I'd expect an exception that is describing the cause of the error, not an "symptom".






[CLJ-1667] Socket test can fail if hard-coded port is unavailable Created: 26/Feb/15  Updated: 26/Feb/15

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

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: io, test

Attachments: Text File socket-test.patch    
Patch: Code
Approval: Triaged

 Description   

I was unable to run the Clojure tests due to this problem. There is a test that hardcodes a port and something else on my machine happened to be using that port.

The patch avoids binding a hard-coded port in the test.

Patch: socket-test.patch

Screened by:



 Comments   
Comment by Andy Fingerhut [ 26/Feb/15 11:31 AM ]

I used to try running the prescreen tests in parallel for two different JDKs on the same machine, and I probably stopped doing that because of this. My use case is a very unusual one, and not a good reason to change this by itself, but my use case certainly made this conflict happen regularly.

Comment by Alex Miller [ 26/Feb/15 11:57 AM ]

No good reason not to fix it! Silly test.





[CLJ-1666] change 'fun' to 'f' in doc strings Created: 22/Feb/15  Updated: 22/Feb/15  Resolved: 22/Feb/15

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

Type: Enhancement Priority: Trivial
Reporter: Patrick Ryan Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: docstring, enhancement

Attachments: File fix-fun-to-f.diff    
Patch: Code

 Description   

fix 'function' term references in core/alter, core/commute, and test/clojure/test_clojure/pprint/test_cl_format.clj for consistency.

Rest of codebase uses 'f' to refer to functions.



 Comments   
Comment by Patrick Ryan [ 22/Feb/15 12:17 PM ]

My first patch, any help/critique welcomed.

Comment by Andy Fingerhut [ 22/Feb/15 1:10 PM ]

It looks like the format of the patch is the one expected, so that is good. Not a big deal, but most people use their actual name and email address to identify themselves, rather than an alias and email address.

I don't know whether this patch is of interest to the Clojure developers or not, but I do know that they will never apply patches written by those who have not signed a Clojure contributor agreement – see http://clojure.org/contributing

I did not see your name on the list there. Were you considering signing the CA?

Comment by Patrick Ryan [ 22/Feb/15 1:19 PM ]

I just signed it about an hour or two ago. Patrick Ryan (phiat99@gmail.com) (phiat on github) Thanks for feedback

Comment by Alex Miller [ 22/Feb/15 1:23 PM ]

Hi Patrick, thanks for navigating the process and submitting the patch! Unfortunately, I don't think this particular change is worth doing so I am going to decline it. Sorry about that and I hope I have not discouraged you on your road to using or contributing to Clojure!

Comment by Patrick Ryan [ 22/Feb/15 1:33 PM ]

No problem Alex! I know its a tiny trivial change, just a small OCD thing when I was looking through docs! Thanks for feedback. I will look for 'bigger fish' to fry





[CLJ-1665] take-nth transducer could be faster without rem Created: 20/Feb/15  Updated: 20/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance
Environment:

Mac OS X 10.10.2, JDK 1.8.0_31


Attachments: Text File CLJ-1665-faster-take-nth-transducer-without-rem.patch    
Patch: Code
Approval: Triaged

 Description   

The take-nth transducer is calling rem on each index, which is relatively expensive compared to a zero? test. It could just count down from N instead as the step size is fixed.



 Comments   
Comment by Steve Miner [ 20/Feb/15 12:34 PM ]

Patch attached. It's about 25% faster on a simple test like:

(time (transduce (take-nth 13) + (range 1e7)))
Comment by Steve Miner [ 20/Feb/15 12:41 PM ]

I didn't worry about (take-nth 0) case, but my patch does give a different result. The current implementation gets a divide by zero error (from rem). My patched version returns just the first element once. The regular collection version returns an infinite sequence of the first element. I doubt anyone expects a sensible answer from the 0 case so I didn't try to do anything special with it.

Comment by Michael Blume [ 20/Feb/15 12:55 PM ]

Nice =)

I would say that the transducer version really ought to match the collection version as closely as possible, but I don't think there's actually a way to write a transducer that transforms a finite sequence into an infinite sequence, so no luck there.

Maybe while we're at it we should change both the transducer and the collection arities to throw on zero?





[CLJ-1664] Inconsistency in overflow-handling between type-hinted and reflective calls Created: 19/Feb/15  Updated: 19/Feb/15

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

Type: Defect Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: numerics, reflection


 Description   
(import 'java.io.DataOutputStream)
(import 'java.io.ByteArrayOutputStream)

(defn- ->bytes
  "Convert a Java primitive to its byte representation."
  [write v]
  (let [output-stream (ByteArrayOutputStream.)
        data-output (DataOutputStream. output-stream)]
    (write data-output v)
    (seq (.toByteArray output-stream))))

(defn int->bytes [n]
  (->bytes 
    #(.writeInt ^DataOutputStream %1 %2)
    n))

(defn int->bytes-ref [n]
  (->bytes 
    #(.writeInt %1 %2)
    n))

user=> (int->bytes 5)
(0 0 0 5)
user=> (int->bytes-ref 5)
(0 0 0 5)
user=> (int->bytes (inc Integer/MAX_VALUE))

IllegalArgumentException Value out of range for int: 2147483648  clojure.lang.RT.intCast (RT.java:1115)
user=> (int->bytes-ref (inc Integer/MAX_VALUE))
(-128 0 0 0)

So it looks like type-hinting the DataOutputStream results in bytecode calling RT.intCast, which throws because the value is too large. In the reflective case, we locate the method writeInt at runtime, and then do not call RT.intCast, but instead allow the long to be downcast without bounds checking.

It seems like we should be calling RT.intCast in both cases?






[CLJ-1663] DynamicClassLoader delegates to parent classloader before checking in its URL list Created: 18/Feb/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: classloader, regression

Attachments: Text File 0001-CLJ-1663-delegate-loadClass-to-super-classloader-bef.patch    
Patch: Code
Approval: Ok

 Description   

See Cursive #748. Cursive calls into Leiningen in-process, and before doing that it creates a new DynamicClassLoader which uses an IntelliJ PluginClassLoader as its parent. This is throwing a CNFE, although the URL containing the class is present in the DynamicClassLoader URL list.

Cause: The patch for CLJ-979 added an implementation of loadClass that delegates to "getParent().loadClass()", which in this case delegates to PluginClassLoader.loadClass(). This was incorrect as the current implementation should have been to delegate to the loadClass method of the superclass, which will take care of delegating to the loadClass method of the parent class loader if necessary.

Approach: The proposed patch replaces the call to "getParent().loadClass()" with a call "super.loadClass()" fixing this issue.

Screened by: Alex Miller



 Comments   
Comment by Colin Fleming [ 18/Feb/15 6:25 AM ]

Unfortunately getClassLoadingLock(name) is only available from Java 1.7+ and I am targeting Java 1.6. However reverting that part of the patch to synchronize on "this" as previously does indeed fix the original problem.

Comment by Nicola Mometto [ 18/Feb/15 7:22 AM ]

I'll revert the getClassLoadingLock change then, it was actually out of scope for this ticket.





[CLJ-1662] folding over hash-map nested hash-map throws exception Created: 17/Feb/15  Updated: 17/Feb/15

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: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers
Environment:

JVM 1.7.0_76



 Description   

I got a baffling exception in a recursive function that folds. REPL transcript below:

nREPL server started on port 57818 on host 127.0.0.1 - nrepl://127.0.0.1:57818
REPL-y 0.3.5, nREPL 0.2.6
Clojure 1.7.0-alpha5
Java HotSpot(TM) 64-Bit Server VM 1.7.0_76-b13
    Docs: (doc function-name-here)
          (find-doc "part-of-name-here")
  Source: (source function-name-here)
 Javadoc: (javadoc java-object-or-class-here)
    Exit: Control+D or (exit) or (quit)
 Results: Stored in vars *1, *2, *3, an exception in *e

user=> (use 'foldtest.core)
nil
user=> (source leafs)
(defn leafs [xs]
  (->> (r/mapcat (fn [k v]
                   (if (map? v)
                     (leafs v)
                     [[k v]])) xs)
       (r/foldcat)))
nil
user=> (leafs (hash-map :a (hash-map :b 1 :c 2)))

ClassCastException clojure.lang.PersistentHashMap$1 cannot be cast to clojure.lang.IFn  clojure.core.reducers/fjinvoke (reducers.clj:48)
user=> (pst)
ClassCastException clojure.lang.PersistentHashMap$1 cannot be cast to clojure.lang.IFn
	clojure.core.reducers/fjinvoke (reducers.clj:48)
	clojure.lang.PersistentHashMap.fold (PersistentHashMap.java:207)
	clojure.core.reducers/eval1347/fn--1348 (reducers.clj:367)
	clojure.core.reducers/eval1220/fn--1221/G--1211--1232 (reducers.clj:81)
	clojure.core.reducers/folder/reify--1247 (reducers.clj:130)
	clojure.core.reducers/fold (reducers.clj:98)
	clojure.core.reducers/fold (reducers.clj:96)
	clojure.core.reducers/foldcat (reducers.clj:318)
	foldtest.core/leafs (core.clj:5)
	foldtest.core/leafs/fn--1367 (core.clj:7)
	clojure.core.reducers/mapcat/fn--1277/fn--1280 (reducers.clj:185)
	clojure.lang.PersistentHashMap$NodeSeq.kvreduce (PersistentHashMap.java:1127)
nil
user=>

Note that it must be a hash-map nested in a hash-map. Other combinations of array and hash maps seem fine:

user=> (leafs (array-map :a (hash-map :b 1 :c 2)))
[[:c 2] [:b 1]]
user=> (leafs (hash-map :a (array-map :b 1 :c 2)))
[[:b 1] [:c 2]]
user=> (leafs (hash-map :a (hash-map :b 1 :c 2)))

ClassCastException clojure.lang.PersistentHashMap$1 cannot be cast to clojure.lang.IFn  clojure.core.reducers/fjinvoke (reducers.clj:48)
user=> (leafs (array-map :a (array-map :b 1 :c 2)))
[[:b 1] [:c 2]]
user=>

Possibly related: CLJCLR-63

It took me a while to discover this because of this inconsistency (which I am not sure is a bug):

user=> (def a {:a 1})
#'user/a
user=> (type a)
clojure.lang.PersistentHashMap
user=> (let [a {:a 1}] (type a))
clojure.lang.PersistentArrayMap
user=> (type {:a 1})
clojure.lang.PersistentArrayMap
user=>

(I had put test input in a def, but using the defed var always failed but literals always worked!)






[CLJ-1661] Varargs protocol impls can be defined but not called Created: 17/Feb/15  Updated: 19/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Reno Reckling Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-1661-v1.patch    
Patch: Code

 Description   

The compiler accepts this:

(deftype foo []
clojure.lang.IFn
(invoke [this & xs]))

However calling ((foo.) :bar) will throw an AbstractMethodError. Wouldn't some checking be desirable?



 Comments   
Comment by Reno Reckling [ 17/Feb/15 11:09 AM ]

This is a clone of http://dev.clojure.org/jira/browse/CLJ-1024 because the original with its attached patches was forgotten with the reason that "It has to wait and cannot be applied in 1.5" which is 2 major versions ago now, with 1.7 underway.

I would like to reopen it, or continue working on it in this ticket because i just stumbled over this issue the second time and the debugging sessions that follow this are annoying.

Comment by Andy Fingerhut [ 19/Feb/15 12:23 PM ]

Fix Version/s was Release 1.5, but that field should only be set by Clojure screeners.

Comment by Reno Reckling [ 19/Feb/15 12:41 PM ]

Yes, i just cloned the original issue. Later i realized that I'm unable to edit any of the fields.
The issue is just concerned with a missing warning/error when trying to compile protocols with "&" in the argument list as they are interpreted as a variable name "&" instead of a varargs placeholder which the user probably expects.

Comment by Michael Blume [ 19/Feb/15 2:17 PM ]

Here's a forward-port of the 1024 patch





[CLJ-1660] Unify Stepper and MultiStepper in LazyTransformer Created: 16/Feb/15  Updated: 16/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-1660-v1.patch    
Patch: Code

 Description   

This seemed worthwhile to me mainly because some of the stepper logic is actually pretty fiddly, so it seems better not to have it duplicated.






[CLJ-1659] compile leaks files Created: 16/Feb/15  Updated: 16/Feb/15

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

Type: Defect Priority: Major
Reporter: Ralf Schmitt Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Approval: Triaged

 Description   

clojure's compile function leaks file descriptors, i.e. it relies on garbage collection to close the files. I'm trying to use boot [1] on windows and ran into the problem, that files could not be deleted intermittently [2]. The problem is that clojure's compile function, or rather clojure.lang.RT.lastModified relies on garbage collection to close files. lastModified looks like:

static public long lastModified(URL url, String libfile) throws IOException{
	if(url.getProtocol().equals("jar")) {
		return ((JarURLConnection) url.openConnection()).getJarFile().getEntry(libfile).getTime();
	}
	else {
		return url.openConnection().getLastModified();
	}
}

Here's the stacktrace from file leak detector [3]:

#205 C:\Users\ralf\.boot\tmp\Users\ralf\home\steinmetz\2mg\-x24pa9\steinmetz\fx\config.clj by thread:clojure-agent-send-off-pool-0 on Sat Feb 14 19:58:46 UTC 2015
    at java.io.FileInputStream.(FileInputStream.java:139)
    at java.io.FileInputStream.(FileInputStream.java:93)
    at sun.net.www.protocol.file.FileURLConnection.connect(FileURLConnection.java:90)
    at sun.net.www.protocol.file.FileURLConnection.initializeHeaders(FileURLConnection.java:110)
    at sun.net.www.protocol.file.FileURLConnection.getLastModified(FileURLConnection.java:178)
    at clojure.lang.RT.lastModified(RT.java:390)
    at clojure.lang.RT.load(RT.java:421)
    at clojure.lang.RT.load(RT.java:411)
    at clojure.core$load$fn__5066.invoke(core.clj:5641)
    at clojure.core$load.doInvoke(core.clj:5640)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.core$load_one.invoke(core.clj:5446)
    at clojure.core$compile$fn__5071.invoke(core.clj:5652)
    at clojure.core$compile.invoke(core.clj:5651)
    at pod$eval52.invoke(NO_SOURCE_FILE:0)
    at clojure.lang.Compiler.eval(Compiler.java:6703)
    at clojure.lang.Compiler.eval(Compiler.java:6693)
    at clojure.lang.Compiler.eval(Compiler.java:6666)
    at clojure.core$eval.invoke(core.clj:2927)
    at boot.pod$eval_in_STAR_.invoke(pod.clj:203)
    at clojure.lang.Var.invoke(Var.java:379)
    at org.projectodd.shimdandy.impl.ClojureRuntimeShimImpl.invoke(ClojureRuntimeShimImpl.java:88)
    at org.projectodd.shimdandy.impl.ClojureRuntimeShimImpl.invoke(ClojureRuntimeShimImpl.java:81)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:483)
    at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:93)
    at clojure.lang.Reflector.invokeInstanceMethod(Reflector.java:28)
    at boot.pod$eval_in_STAR_.invoke(pod.clj:206)
    at boot.task.built_in$fn__1417$fn__1418$fn__1421$fn__1422.invoke(built_in.clj:433)
    at boot.task.built_in$fn__1443$fn__1444$fn__1447$fn__1448.invoke(built_in.clj:446)
    at boot.task.built_in$fn__1190$fn__1191$fn__1194$fn__1195.invoke(built_in.clj:232)
    at boot.core$run_tasks.invoke(core.clj:663)
    at clojure.lang.Var.invoke(Var.java:379)
    at boot.user$eval297$fn__298.invoke(boot.user4212477544188689077.clj:33)
    at clojure.core$binding_conveyor_fn$fn__4145.invoke(core.clj:1910)
    at clojure.lang.AFn.call(AFn.java:18)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)

So, it looks like getLastModified opens an InputStream internally. On Stackoverflow [4] there's a discussion on how to close the URLConnection correctly.

On non-Windows operating systems this shouldn't be much of a problem. But on windows this hurts very much, since you can't delete
files that are opened by some process.

I've tested with clojure 1.6.0, but I assume other version are also affected.

[1] http://boot-clj.com/
[2] https://github.com/boot-clj/boot/issues/117
[3] http://file-leak-detector.kohsuke.org/
[4] http://stackoverflow.com/questions/9150200/closing-urlconnection-and-inputstream-correctly






[CLJ-1658] Redefine doseq in terms of reduce Created: 13/Feb/15  Updated: 13/Feb/15  Resolved: 13/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None

Attachments: File doseq-with-reduce.diff    
Patch: Code

 Description   

The current version of doseq calls seq on each input collection in order to obtain a unified interface. If however, the input collection is not a seq, this will cause a fair amount of unnecessary allocation. By modifying doseq to use reduce internally we can not only reduce the amount of code doseq produces but the resulting code is also much faster due to reduction being handed off to the collection itself. The net effect is about a 3x performance boost for vectors, with no impact on code for seqs. 

Approach: we re-define doseq in core.clj after reduce has been defined.

Benchmarks:

Before:

user=> (def v (vec (range (* 1024 1024))))
#'user/v
user=> (dotimes [x 100] (time (doseq [x v] x)))

average: 6ms

user=>  (def s (doall (range (* 1024 1024))))
#'user/s
user=> (dotimes [x 100] (time (doseq [x (seq v)] x)))
average: 2ms

After:

user=> (def v (vec (range (* 1024 1024))))
#'user/v
user=> (dotimes [x 100] (time (doseq [x v] x)))

average: 1.5ms

user=>  (def s (doall (range (* 1024 1024))))
#'user/s
user=> (dotimes [x 100] (time (doseq [x s] x)))
average: 2ms

Notes: All existing doseq tests pass, so no new tests were added.



 Comments   
Comment by Alex Miller [ 13/Feb/15 10:25 AM ]

Summarize benchmark tests and results in description?

Comment by Ghadi Shayban [ 13/Feb/15 10:32 AM ]

dupe of CLJ-1322

Comment by Timothy Baldridge [ 13/Feb/15 10:37 AM ]

well, shoot.





[CLJ-1657] proxy creates bytecode that calls super methods of abstract classes Created: 08/Feb/15  Updated: 08/Feb/15

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

Type: Defect Priority: Minor
Reporter: Alexander Yakushev Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Everywhere, but so far relevant only on Android 5.0



 Description   

When proxy is used to extend abstract classes (e.g. java.io.Writer), the bytecode it produces include the call to non-existing super methods. For example, here's decompiled method from clojure/pprint/column_writer.clj:

public void close()
    {
        Object obj;
label0:
        {
            obj = RT.get(__clojureFnMap, "close");
            if(obj == null)
                break label0;
            ((IFn)obj).invoke(this);
            break MISSING_BLOCK_LABEL_31;
        }
        JVM INSTR pop ;
        super.close();
    }

As you can see on the last line, super.close() tries to call a non-defined method (because close() is abstract in Writer).

This hasn't been an issue anywhere until Android 5.0 came out. Its bytecode optimizer is very aggressive and rejects such code. Google guys claim that it is a bug in their code, which they already fixed[1]. Still I wonder if having faulty bytecode, that is not valid by Java standards, might cause issues in future (not only on Android, but in other enviroments too).

[1] https://code.google.com/p/android/issues/detail?id=80687






[CLJ-1656] Unroll assoc and assoc! for small numbers of arguments Created: 06/Feb/15  Updated: 23/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Tom Crayford Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: performance

Attachments: File assoc.diff     Text File CLJ-1656-v1.patch     Text File CLJ-1656-v2.patch     Text File CLJ-1656-v3.patch     Text File CLJ-1656-v4.patch     Text File CLJ-1656-v5.patch     File cpuinfo     File javaversion     File output     File uname    
Patch: Code
Approval: Triaged

 Description   

Whilst doing performance work recently, I discovered that unrolling to single assoc calls were significantly faster than using multiple keys (~10% for my particular application). Zachary Tellman then pointed out that clojure.core doesn't unroll assoc at all, even for the case of relatively low numbers of keys.

We already unroll other performance critical functions that call things via `apply`, e.g. `update` https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L5914, but `assoc` (which is, I think in the critical path for quite a bunch of applications and libraries), would likely benefit from this.

I have not yet developed patches for this, but I did some standalone benchmarking work:

https://github.com/yeller/unrolling-assoc-benchmarks

benchmark results:

code: https://github.com/yeller/unrolling-assoc-benchmarks/blob/master/src/bench_assoc_unrolling.clj

  1 2 3 4
empty array map (not unrolled) 23ns 93ns 156ns 224ns
empty array map (unrolled assoc) N/A 51ns 80ns 110ns
         
20 element persistent hashmap (not unrolled) 190ns 313ns 551ns 651ns
20 element persistent hashmap (unrolled assoc) N/A 250ns 433ns 524ns
         
record (not unrolled) 12ns 72ns 105ns 182ns
record (unrolled assoc) N/A 21ns 28ns 41ns

Each measurement was made in a separate JVM, to avoid JIT path dependence.

Benchmarks were ran on a commodity server (8 cpus, 32gb ram), with ubuntu 12.04 and a recent release of Java 8. Attached are `cpuinfo`, `uname` and `java -version` output.

Relatively standard JVM production flags were enabled, and care was taken to disable leiningen's startup time optimizations (which disable many of the JIT optimizations).

Benchmarks can be run by cloning the repository, and running `script/bench`

There's one outstanding question for this patch: How far should we unroll these calls? `update` (which is unrolled in the 1.7 alphas) is unrolled to 3 arguments. Adding more unrolling isn't difficult, but it does impact the readability of assoc.



 Comments   
Comment by Tom Crayford [ 09/Feb/15 12:01 PM ]

Ok, attached `assoc.diff`, which unrolls this to a single level more than the current code (so supporting two key/value pairs without recursion). The code's going to get pretty complex in the case with more than the unrolled number of keys if we continue on this way, so I'm unsure if this is a good approach, but the performance benefits seem very compelling.

Comment by Michael Blume [ 09/Feb/15 3:35 PM ]

Since the unroll comes out kind of hairy, why not have a macro write it for us?

Comment by Michael Blume [ 09/Feb/15 4:03 PM ]

Patch v2 includes assoc!

Comment by Tom Crayford [ 09/Feb/15 5:01 PM ]

I benchmarked conj with similar unrolling, across a relatively wide range of datatypes from core (lists, sets, vectors, each one empty and then again with 20 elements):

  1 2 3 4
empty vector (not unrolled) 19ns 57ns 114ns 126ns
empty vector (unrolled conj) N/A 44ns 67ns 91ns
         
20 element vector (not unrolled) 27.35ns 69ns 111ns 107ns
20 element vector (unrolled conj) N/A 54ns 79ns 104ns
         
empty list (not unrolled) 7ns 28ns 53ns 51ns
empty list (unrolled conj) N/A 15ns 20ns 26ns
         
twenty element list (not unrolled) 8.9ns 26ns 49ns 49ns
twenty element list (unrolled) N/A 15ns 19ns 30ns
         
empty set (not unrolled) 64ns 170ns 286ns 290ns
empty set (unrolled) N/A 154ns 249ns 350ns
         
twenty element set (not unrolled) 33ns 81ns 132ns 130ns
twenty element set (unrolled) N/A 69ns 108ns 139ns

Benchmarks were run on the same machine as before. There's a less clear benefit here, except for lists, where the overhead of creating seqs and recursing seems to be clearly dominating the cost of actually doing the conj (which makes sense - conj on any element list should be a very cheap operation). Raw benchmark output is here: https://gist.github.com/tcrayford/51a3cd24b8b0a8b7fd74

Comment by Tom Crayford [ 09/Feb/15 5:04 PM ]

Michael Blume: I like those patches! They read far nicer to me than my original patch. Did you check if any of those macro generated methods blew Hotspot's hot code inlining limit? (it's 235 bytecodes). That'd be my only worry with using macros here - it's easy to generate code that defeats the inliner.

Comment by Michael Blume [ 09/Feb/15 5:57 PM ]

Thanks! This is new for me, so I might be doing the wrong thing, but I just ran nodisassemble over both definitions and the "instruction numbers" next to each line go up to 219 for the varargs arity assoc and up to 251 for assoc!, so, assuming I'm looking at the right thing, maybe that one needs to have an arity taken off? If I remove the highest arity I get 232 for varargs which is just under the line.

I guess alternatively we could call assoc! instead of assoc!* in the varargs arity, which removes a lot of code – in that case it's 176 for varargs and 149 for six pairs.

Comment by Michael Blume [ 09/Feb/15 6:01 PM ]

Gah, I forgot to include coll in the varargs call to assoc!

which reminds me that this patch needs tests.

Comment by Michael Blume [ 09/Feb/15 10:27 PM ]

OK, this has some fixes I made after examining the disassembled output. There's a change to the assoc!* macro to be sure it type-hints correctly – I'm honestly not sure why it didn't type-hint properly before, but it does now. Also, I changed the call to assoc! rolling up the first six entries at the top of the varargs version from a macro call to a function call so it'd fit within the 251 inlineable bytecodes. (This, again, is assuming I'm reading the output correctly).

Comment by Tom Crayford [ 10/Feb/15 6:38 AM ]

Michael: Wanna push a branch with these patches to clojars or something? Then I can rerun the benchmarks with the exact code in the patches.

Comment by Michael Blume [ 10/Feb/15 2:36 PM ]

Hmm, not sure I know how to do that – here's a branch on github though https://github.com/MichaelBlume/clojure/tree/unroll-assoc

Comment by Michael Blume [ 12/Feb/15 1:12 PM ]

v5 marks the helper macros private.

Comment by Tom Crayford [ 13/Feb/15 4:11 AM ]

Michael: was that branch just based off clojure/clojure master? I tried running benchmarks off it, but ran into undefined var errors when building this code (which doesn't happen with alpha5):

(Retrieving com/yellerapp/clojure-unrolled-assoc/1.7.0-unrollassoc-SNAPSHOT/clojure-unrolled-assoc-1.7.0-unrollassoc-20150213.092242-1.pom from clojars)
(Retrieving com/yellerapp/clojure-unrolled-assoc/1.7.0-unrollassoc-SNAPSHOT/clojure-unrolled-assoc-1.7.0-unrollassoc-20150213.092242-1.jar from clojars)
(Retrieving org/clojure/clojure/1.3.0/clojure-1.3.0.jar from central)
Exception in thread "main" java.lang.RuntimeException: Unable to resolve symbol: bench in this context, compiling:(bench_assoc_unrolling.clj:5)
at clojure.lang.Compiler.analyze(Compiler.java:6235)
at clojure.lang.Compiler.analyze(Compiler.java:6177)
at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3452)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6411)
at clojure.lang.Compiler.analyze(Compiler.java:6216)
at clojure.lang.Compiler.analyze(Compiler.java:6177)
at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5572)
at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5008)

Comment by Michael Blume [ 23/Feb/15 5:08 PM ]

Ok, how are you building? Why the strange clojure group?

Comment by Michael Blume [ 23/Feb/15 5:09 PM ]

The existing version of assoc does runtime checking that an even number of varargs are passed in, but assoc! does not. Do we want to preserve this behavior or do checks in both?

Comment by Michael Blume [ 23/Feb/15 6:00 PM ]

Also, I'm curious how relevant inlining is here – does HotSpot inlining actually work with Var invocation when there's a getRootBinding step in the way?

Comment by Alex Miller [ 23/Feb/15 7:59 PM ]

Yes, inlining works through var invocation.





[CLJ-1655] Dorun's behavior when called with two argument's is both unintuitive and undocumented. Created: 04/Feb/15  Updated: 04/Feb/15

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

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


 Description   

Dorun can be called as (dorun n coll). When called this way, dorun will force n+1 elements from coll, which seems unintuitive. I can't necessarily call this a defect, though. It doesn't deviate from the documented behavior because there is no documented behavior – the two-argument arity is not mentioned in the docstring.

user=> (defn printing-range [n] (lazy-seq (println n) (cons n (printing-range (inc n)))))
#'user/printing-range
user=> (dorun 0 (printing-range 1))
1
nil
user=> (dorun 3 (printing-range 1))
1
2
3
4
nil





[CLJ-1654] Reuse seq in some Created: 04/Feb/15  Updated: 04/Feb/15

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

Type: Enhancement Priority: Trivial
Reporter: Gijs Stuurman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: Text File 0000-reuse-seq-in-some.patch    
Patch: Code
Approval: Triaged

 Description   

By using when-let at most two seq constructions can be avoided per invocation of some.



 Comments   
Comment by Ghadi Shayban [ 04/Feb/15 12:11 PM ]

This is similar to the tweak to dorun/doall in CLJ-1515. It is a good benefit when a collection doesn't cache its seq





[CLJ-1653] str of an empty list is not "()" Created: 02/Feb/15  Updated: 03/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, print

Attachments: Text File CLJ-1653-toString-for-EmptyList.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The str of an empty list is surprisingly not "()". This is inconsistent with the result for the empty map {} or empty vector (). It would be convenient if `(str ())` returned "()". The work-around is to use `pr-str`, which is arguably the "correct" thing to do. However, there doesn't seem to be any reason that Clojure couldn't return "()".

(str ())
;=> "clojure.lang.PersistentList$EmptyList@1"

(str {} [] ())
;=> "{}[]clojure.lang.PersistentList$EmptyList@1"

;; Work-around: use `pr-str` instead of `str`

(pr-str () {} [])
"() {} []"


 Comments   
Comment by Steve Miner [ 02/Feb/15 3:30 PM ]

PersistentList$EmptyList should have a toString method that returns "()".

Comment by Steve Miner [ 02/Feb/15 3:45 PM ]

add toString() for EmptyList

Comment by Steve Miner [ 02/Feb/15 3:45 PM ]

patch and test for toString() method on EmptyList.

Comment by Nicola Mometto [ 02/Feb/15 4:03 PM ]

Not sure how this is different from

user=> (str (range 10))
"clojure.lang.LazySeq@9ebadac6"

pr-str works fine on both () and (range 10) btw.

Comment by Steve Miner [ 02/Feb/15 5:09 PM ]

I agree in principle that pr-str is the right thing to use. I will counter the Slippery Slope argument by invoking the Principle of Least Astonishment. My argument for the proposed patch is that () is a common value and the current behavior is inconsistent with similar empty values, {} and []. I think it would be convenient and useful, especially for beginners, to fix just this one case of the empty list. On the other hand, it's a minor issue so I won't push it.

Comment by Michael Blume [ 02/Feb/15 5:59 PM ]
user=> (str '())
"clojure.lang.PersistentList$EmptyList@1"
user=> (str '(1 2))
"(1 2)"

This really makes empty list seem like a special case.





[CLJ-1652] clojure.test counter reports are not thread safe Created: 30/Jan/15  Updated: 30/Jan/15  Resolved: 30/Jan/15

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

Type: Defect Priority: Major
Reporter: David Sargeant Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None

Attachments: File test-thread-safety.diff    
Patch: Code

 Description   

The update to the

*report-counters*
ref in the inc-report-counter function is not atomic and does not produce the expected number of assertions when tests are run on multiple threads.

Failure example:

(use 'clojure.test)

(deftest test-counter-parallel
  (doall (pmap (fn [x] (is true)) (range 1000))))
  
(run-tests)

Ran 1 tests containing 183 assertions.
0 failures, 0 errors.
{:type :summary, :fail 0, :error 0, :pass 183, :test 1}

Same example, but single-threaded:

(use 'clojure.test)

(deftest test-counter
  (doall (map (fn [x] (is true)) (range 1000))))
  
(run-tests)

Ran 1 tests containing 1000 assertions.
0 failures, 0 errors.
{:type :summary, :fail 0, :error 0, :pass 1000, :test 1}

The attached patch fixes the issue.



 Comments   
Comment by Alex Miller [ 30/Jan/15 12:40 PM ]

Dupe of CLJ-1528 I think.





[CLJ-1651] Erroneous error message when using into to create a map. Created: 29/Jan/15  Updated: 29/Jan/15

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

Type: Defect Priority: Minor
Reporter: Justin Glenn Smith Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting


 Description   

If you provide a sequence instead of a vector type for the entries provided to into for creating a hash-map, the error message is misleading.

org.noisesmith.orsos=> (into {} '((:a 0) (:b 1)))

ClassCastException clojure.lang.Keyword cannot be cast to java.util.Map$Entry clojure.lang.ATransientMap.conj (ATransientMap.java:44)

As we see, it reports the type of the first item in the entry, rather than the actual error, the type of the entry itself, which can be particularly confusing if the key in the entry is actually a valid type to be an entry:

=> (into {} '((["a" 1] ["b" 2]) (["c" 3] ["d" 4])))

ClassCastException clojure.lang.PersistentVector cannot be cast to java.util.Map$Entry clojure.lang.ATransientMap.conj (ATransientMap.java:44)






[CLJ-1650] compile forces namespace reloading from AOT classfile Created: 29/Jan/15  Updated: 20/Feb/15

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: regression

Attachments: Text File 0001-CLJ-1650-compile-forces-namespace-reloading-from-AOT.patch    
Patch: Code
Approval: Vetted

 Description   

The patch for CLJ-979 exposed an issue with how clojure.core/compile is implemented, which causes the bug reported here: https://groups.google.com/d/msg/clojure-dev/jj87-4yVlWI/YKG4QazhPuAJ

The cause of this regression is that clojure.core/compile doesn't take into account clojure.core/loaded-libs, causing

(binding [*compile-files* true] (require 'some.ns))
(compile 'some.ns)

to reload 'some-ns from the AOT class with the call to compile.

Since the AOT compiled namespace is not loaded by DynamicClassLoader but using the underlying java.net.URLClassLoader, code that relies on deftypes will have references to the AOT versions hardcoded, breaking the class loading policy introduced with CLJ-979 of prefering the in-memory versions to the AOT ones.

The fix for this, as implemented in the attached patch, is to make compile loaded-libs-aware, so that it won't force any namespace re-loading when unnecessary.






[CLJ-1649] Hash/equality inconsistency for floats & doubles Created: 23/Jan/15  Updated: 23/Jan/15

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

Type: Defect Priority: Minor
Reporter: Michael Gardner Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: numerics


 Description   

This is closely related to CLJ-1036, but there was a suggestion to make a new ticket.

The issue is that for a float f and a double d, we can have (= f d) but not (= (hash f) (hash d)), which breaks a fundamental assumption about hash/equality consistency and leads to weirdness like this (from Immo Heikkinen's email to the Clojure mailing list):

(= (float 0.5) (double 0.5))
=> true
(= #{(float 0.5)} #{(double 0.5)})
=> true
(= {:a (float 0.5)} {:a (double 0.5)})
=> true
(= #{{:a (float 0.5)}} #{{:a (double 0.5)}})
=> false

One way to resolve this would be to tweak the hashing of floats and/or doubles, but that suggestion has apparently been rejected.

An alternative would be to modify = so that it never returns true for float/double comparisons. One should never compare floats with doubles using = anyway, so such a change should have minimal impact beyond restoring hash/equality consistency.






[CLJ-1648] Use equals() instead of == when resolving Symbol Created: 22/Jan/15  Updated: 23/Jan/15

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

Type: Defect Priority: Minor
Reporter: Steven Yi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: Compiler

Attachments: File resolve-symbol-equals.diff    
Patch: Code
Approval: Triaged

 Description   

In Compiler.java, resolveSymbol() uses == to compare a Symbol's ns and the found namespace's name. This can result in a false comparison result, though the name's may be equal. In the following example:

ond.core=> (require '[clojure.string])
nil
ond.core=> `(clojure.string/join "," [1 2])
false : true
nil

The symbol resolution of clojure.string/join within the syntax quote is false by ==, but true by equals(). (The "false : true" above is reported from System.out.println code I had put into Compiler.java while testing.) The result is that a new Symbol is allocated, when the previous one should be returned.

As noted by Alex in this clojure-dev thread[1]:

"Prior to Clojure 1.7, Symbol name and ns were interned so == would actually have worked, but that is no longer the case."

Attached is a patch resolve-symbol-equals.diff that modifies the comparison to use equals().

[1] - https://groups.google.com/forum/#!topic/clojure-dev/58fYUSIEfxg






[CLJ-1647] infinite loop in 'partition' and 'partition-all' when 'step' or 'n' is not positive Created: 20/Jan/15  Updated: 03/Feb/15

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

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

Approval: Triaged

 Description   

If you pass a non-positive value of 'n' or 'step' to partition, you get an infinite loop. Here are a few examples:

(partition 0 [1 2 3])
(partition 1 -1 [1 2 3])

To fix this, I recommend adding 'assert-args' to the appropriate places in partition and partition-all:

(assert-args
(pos? n) "n must be positive"
(pos? step) "step must be positive" )



 Comments   
Comment by Alex Miller [ 03/Feb/15 5:34 PM ]

Also see CLJ-764





[CLJ-1646] Small filter performance enhancement Created: 19/Jan/15  Updated: 01/Feb/15

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: performance

Attachments: Text File clj-1646.patch    
Patch: Code
Approval: Triaged

 Description   

I found when working on other tickets for 1.7 that filter repeats each call to .nth on the chunk.
Reusing the result is a small perf boost for all filter uses.

Timing with criterium quick-bench:

What stock patch applied comment
(into [] (filter odd? (range 1024))) 54.3 µs 50.2 µs 7.5% reduction

Approach: storing the nth value as to avoid double lookup.

Patch: clj-1646.patch
Screened by:






[CLJ-1645] 'javap -v' on protocol class reveals no source file Created: 16/Jan/15  Updated: 08/Feb/15

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

Type: Defect Priority: Minor
Reporter: Fabio Tudone Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, protocols, source
Environment:

Mac OS X Yosemite.

java version "1.8.0_25"
Java(TM) SE Runtime Environment (build 1.8.0_25-b17)
Java HotSpot(TM) 64-Bit Server VM (build 25.25-b02, mixed mode)


Attachments: Text File CLJ-1645-protocol-class-has-no-source-file-information.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Through "javap -v" I can find source filename information in Clojure-generated datatype class files but not in protocol ones.






[CLJ-1644] into-array fails for sequences starting with nil Created: 15/Jan/15  Updated: 15/Jan/15

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

Type: Defect Priority: Major
Reporter: Michael Blume Assignee: Michael Blume
Resolution: Unresolved Votes: 0
Labels: arrays

Attachments: Text File CLJ-1644-array-first-nil-v1.patch    
Patch: Code and Test
Approval: Triaged

 Description   

(into-array [nil 1 2]) => NullPointerException

Found during work on CLJ-1643



 Comments   
Comment by Michael Blume [ 15/Jan/15 1:45 PM ]

uploading patch v1 (adding nil as a cons-able element in CLJ-1643 will also bring this out, but I don't want to make the one patch depend on the other)





[CLJ-1643] Generative test for sequence implementations Created: 15/Jan/15  Updated: 15/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Michael Blume Assignee: Michael Blume
Resolution: Unresolved Votes: 0
Labels: generative-test, test

Attachments: Text File clj-1643-gen-seq-test-v1.patch    
Patch: Code and Test
Approval: Triaged

 Description   

This is an attempt to write a minimal-foreknowledge failing test for CLJ-1633. By minimal-foreknowledge, I mean a test that fails in the presence of the bug, but which one could imagine writing without intimate knowledge of the details of the bug. I suspect that looking for tests like this is a good way to find gaps in test coverage, and produce tests that will uncover novel regressions later on.

Approach: Generate a single list of operations that could be performed on a sequence, changing that sequence. Make two copies of that operation list, and insert what should be identity-preserving operations into each. Run the two lists of operations and verify that the final results are the same.

With CLJ-1633 unfixed, we get this output:

[java] Testing clojure.test-clojure.sequences
     [java]
     [java] FAIL in (seq-gentest) (sequences.clj:135)
     [java] {:acts1 (->> nil (cons :foo) (cons :foo) into-array next (apply list)),
     [java]  :acts2 (->> nil (cons :foo) (cons :foo) next),
     [java]  :result1 (:foo :foo),
     [java]  :result2 (:foo),
     [java]  :pass false}
     [java]
     [java] expected: (:result res)
     [java]   actual: false





[CLJ-1642] Add mention of new :warn-on-boxed option to doc string of unchecked-math Var Created: 15/Jan/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring

Attachments: Text File clj-1642-v1.patch    
Patch: Code
Approval: Ok

 Description   

A small doc string enhancement about the new compiler behavior in Clojure 1.7 when *unchecked-math* is bound to :warn-on-boxed

https://github.com/clojure/clojure/blob/master/changes.md#13-warn-on-boxed-math

Patch: clj-1642-v1.patch
Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 15/Jan/15 11:51 AM ]

Patch clj-1642-v1.patch dated Jan 15 2014 is one way to document the new :warn-on-boxed behavior.





[CLJ-1641] AOT compilation fails for a kind of circular dependency after CLJ-1544 patch Created: 14/Jan/15  Updated: 16/Jan/15  Resolved: 16/Jan/15

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

Type: Defect Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: regression

Attachments: Text File 0001-CLJ-1641-disallow-circular-dependencies-even-if-the-.patch    

 Description   

I am assuming the summary and description will be updated from this initial version. This may not be a bug, but simply the way things are after CLJ-1544 is fixed, and AOT compilation of some kinds of cyclic namespace dependencies not throwing an exception in earlier versions of Clojure was an accident.

Behavior before CLJ-1544 patch applied:

% cat project.clj 
(defproject manifold-cycle "0.1.0-SNAPSHOT"
  :dependencies [[org.clojure/clojure "1.7.0-alpha4"]
                 [manifold "0.1.0-alpha4"]]
  :profiles {:uberjar {:aot :all}})

% cat src/manifold_cycle/core.clj 
(ns manifold-cycle.core
  (:require [manifold.stream :as s]))

% lein version
Leiningen 2.5.0 on Java 1.7.0_45 Java HotSpot(TM) 64-Bit Server VM
% lein clean
% lein uberjar
Compiling manifold-cycle.core
Created /Users/jafinger/clj/glosscycle/target/manifold-cycle-0.1.0-SNAPSHOT.jar
Created /Users/jafinger/clj/glosscycle/target/manifold-cycle-0.1.0-SNAPSHOT-standalone.jar

Behavior after CLJ-1544 patch applied, e.g. by changing Clojure version to 1.7.0-alpha5:

% lein clean
% lein uberjar
Compiling manifold-cycle.core
java.lang.Exception: Cyclic load dependency: [ /manifold/stream ]->/manifold/stream/graph->[ /manifold/stream ]->/manifold_cycle/core, compiling:(manifold/stream/graph.clj:1:1)
	at clojure.core$throw_if.doInvoke(core.clj:5612)
	at clojure.lang.RestFn.invoke(RestFn.java:442)
	at clojure.core$check_cyclic_dependency.invoke(core.clj:5763)
	at clojure.core$load.doInvoke(core.clj:5860)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$load_lib$fn__5383.invoke(core.clj:5708)
	at clojure.core$load_lib.doInvoke(core.clj:5707)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5746)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5829)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at manifold.stream.graph$loading__5322__auto____856.invoke(graph.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3600)
	at clojure.lang.Compiler.compile1(Compiler.java:7290)
	at clojure.lang.Compiler.compile1(Compiler.java:7280)
	at clojure.lang.Compiler.compile(Compiler.java:7356)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5436.invoke(core.clj:5863)
	at clojure.core$load.doInvoke(core.clj:5862)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$load_lib$fn__5383.invoke(core.clj:5708)
	at clojure.core$load_lib.doInvoke(core.clj:5707)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5746)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5829)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3600)
	at clojure.lang.Compiler.compile1(Compiler.java:7290)
	at clojure.lang.Compiler.compile(Compiler.java:7356)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5436.invoke(core.clj:5863)
	at clojure.core$load.doInvoke(core.clj:5862)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$load_lib$fn__5383.invoke(core.clj:5708)
	at clojure.core$load_lib.doInvoke(core.clj:5707)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5746)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5829)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at manifold_cycle.core$loading__5322__auto____21.invoke(core.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3600)
	at clojure.lang.Compiler.compile1(Compiler.java:7290)
	at clojure.lang.Compiler.compile1(Compiler.java:7280)
	at clojure.lang.Compiler.compile(Compiler.java:7356)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5436.invoke(core.clj:5863)
	at clojure.core$load.doInvoke(core.clj:5862)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$compile$fn__5441.invoke(core.clj:5874)
	at clojure.core$compile.invoke(core.clj:5873)
	at user$eval9$fn__16.invoke(form-init6042653661846269464.clj:1)
	at user$eval9.invoke(form-init6042653661846269464.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6767)
	at clojure.lang.Compiler.eval(Compiler.java:6757)
	at clojure.lang.Compiler.load(Compiler.java:7194)
	at clojure.lang.Compiler.loadFile(Compiler.java:7150)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$init_opt.invoke(main.clj:279)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.Exception: Cyclic load dependency: [ /manifold/stream ]->/manifold/stream/graph->[ /manifold/stream ]->/manifold_cycle/core
	... 87 more
Exception in thread "main" java.lang.Exception: Cyclic load dependency: [ /manifold/stream ]->/manifold/stream/graph->[ /manifold/stream ]->/manifold_cycle/core, compiling:(manifold/stream/graph.clj:1:1)
	at clojure.core$throw_if.doInvoke(core.clj:5612)
	at clojure.lang.RestFn.invoke(RestFn.java:442)
	at clojure.core$check_cyclic_dependency.invoke(core.clj:5763)
	at clojure.core$load.doInvoke(core.clj:5860)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$load_lib$fn__5383.invoke(core.clj:5708)
	at clojure.core$load_lib.doInvoke(core.clj:5707)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5746)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5829)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at manifold.stream.graph$loading__5322__auto____856.invoke(graph.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3600)
	at clojure.lang.Compiler.compile1(Compiler.java:7290)
	at clojure.lang.Compiler.compile1(Compiler.java:7280)
	at clojure.lang.Compiler.compile(Compiler.java:7356)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5436.invoke(core.clj:5863)
	at clojure.core$load.doInvoke(core.clj:5862)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$load_lib$fn__5383.invoke(core.clj:5708)
	at clojure.core$load_lib.doInvoke(core.clj:5707)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5746)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5829)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3600)
	at clojure.lang.Compiler.compile1(Compiler.java:7290)
	at clojure.lang.Compiler.compile(Compiler.java:7356)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5436.invoke(core.clj:5863)
	at clojure.core$load.doInvoke(core.clj:5862)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$load_lib$fn__5383.invoke(core.clj:5708)
	at clojure.core$load_lib.doInvoke(core.clj:5707)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5746)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5829)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at manifold_cycle.core$loading__5322__auto____21.invoke(core.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3600)
	at clojure.lang.Compiler.compile1(Compiler.java:7290)
	at clojure.lang.Compiler.compile1(Compiler.java:7280)
	at clojure.lang.Compiler.compile(Compiler.java:7356)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5436.invoke(core.clj:5863)
	at clojure.core$load.doInvoke(core.clj:5862)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$compile$fn__5441.invoke(core.clj:5874)
	at clojure.core$compile.invoke(core.clj:5873)
	at user$eval9$fn__16.invoke(form-init6042653661846269464.clj:1)
	at user$eval9.invoke(form-init6042653661846269464.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6767)
	at clojure.lang.Compiler.eval(Compiler.java:6757)
	at clojure.lang.Compiler.load(Compiler.java:7194)
	at clojure.lang.Compiler.loadFile(Compiler.java:7150)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$init_opt.invoke(main.clj:279)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.Exception: Cyclic load dependency: [ /manifold/stream ]->/manifold/stream/graph->[ /manifold/stream ]->/manifold_cycle/core
	... 87 more
Compilation failed: Subprocess failed

It is true that Manifold has a kind of cyclic dependency in its implementation. Namespace manifold.stream's ns form has no require of namespace manifold.stream.graph, but there is an explicit require of the namespace after its ns form here: https://github.com/ztellman/manifold/blob/master/src/manifold/stream.clj#L410

Namespace manifold.stream.graph requires manifold.stream in its ns form: https://github.com/ztellman/manifold/blob/master/src/manifold/stream/graph.clj#L5

Reported by Janne Lemmetti in the Clojure Google group thread announcing Clojure 1.7.0-alpha5's release on Jan 11 2015. He discovered the problem while trying to AOT compile a project that uses the gloss library, which depends upon byte-streams, which depends upon manifold.



 Comments   
Comment by Zach Tellman [ 14/Jan/15 3:46 PM ]

Speaking as the author of the circular dependency, I think what I'm doing here should be allowed. I want to surface vars A and C in a namespace, but C relies on B in another namespace, and B relies on A in my original namespace. Therefore, I was able to do this:

(ns b-ns
(:require [a-ns]))

(def b ...)

(ns a-ns)

(def A ...)

(require 'b-ns)

(def C ...)

The assumption here is that the parts of 'a-ns' that 'b-ns' relies on have been defined once 'b-ns' is required and refers back to 'a-ns'. I was happy to find that this worked, as it meant I didn't have to use my previous approach in these situations, which was to factor out the first half of 'a-ns' into a separate namespace, and then import those vars into 'a-ns' (no one has ever liked import-vars).

If the pre-alpha5 behavior I was relying on was too accidental for everyone's taste, I can get rid of it, but I assert that having some official way to "break" reference loops would be very useful.

Comment by Andy Fingerhut [ 14/Jan/15 5:00 PM ]

Zach, I don't know if it makes any difference to you, but I believe that the CLJ-1544 patch did not break what you are doing in Manifold all of the time, only when AOT compilation is used. That may be significant enough of a use case that your statements still stand.

Comment by Alex Miller [ 14/Jan/15 5:44 PM ]

Pulling this into the 1.7 list just so I'm seeing it, not necessarily implying anything re end result as I haven't looked at it yet.

Comment by Andy Fingerhut [ 15/Jan/15 11:42 AM ]

Added to comments as more background, not necessarily suggesting whether this behavior change is a bug or not: Brief email thread from Oct 2014 in Clojure group between Colin Fleming and Steven (Gilardi?) on why Clojure did not warn about any cyclic dependencies in the Manifold library before: https://groups.google.com/forum/#!topic/clojure/wrVFuCjf0_Y/discussion

Comment by Zach Tellman [ 15/Jan/15 1:41 PM ]

I took another look at my code, and found that the design has changed enough that I no longer "need" circular dependencies, and I could just factor out the shared code to a third namespace. This doesn't need to hold up the 1.7.0 release, I guess, but speaking as someone who uses AOT compilation everywhere, divergent behavior between AOT and standard compilation is worrisome.

Comment by Alex Miller [ 15/Jan/15 2:27 PM ]

One way I can read this is as a sign of the tension between "form as unit of compilation" and "file as code container" where these can be separated in normal source loading but are tangled in AOT.

Comment by Nicola Mometto [ 15/Jan/15 3:52 PM ]

Zach, I agree that having different behaviour between AOT and JIT is wrong.

But I also don't agree that having clojure error out on circular dependencies should be considered a bug, I would argue that the way manifold used to implement the circular dependency between manifold.stream and manifold.stream.graph was a just a hack around lack of validation in require.

My proposal to fix this disparity between AOT and JIT is by making require/use check for circular dependencies before checking for already-loaded namespaces.

This way, both under JIT and AOT code like

(ns foo.a (:require foo.b))
(ns foo.b)
(require 'foo.a)

will fail with a circular depdenency error.

This is what the patch I just attached (0001-CLJ-1641-disallow-circular-dependencies-even-if-the-.patch) does.\

Comment by Alex Miller [ 16/Jan/15 12:59 PM ]

CLJ-1544 is being rolled back in alpha6. I'm not exactly sure what to call the status on this one but we do not plan to take further action on it right now. Any future change for CLJ-1544 will consider this case.





[CLJ-1640] Negating Boolean false is false Created: 13/Jan/15  Updated: 13/Jan/15  Resolved: 13/Jan/15

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

Type: Defect Priority: Major
Reporter: Kuldeep Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: bug
Environment:

Ubuntu 14.04



 Description   

% java -version
java version "1.7.0_71"
Java(TM) SE Runtime Environment (build 1.7.0_71-b14)
Java HotSpot(TM) 64-Bit Server VM (build 24.71-b01, mixed mode)

user=> (not (Boolean. "false"))
false
user=> (not (Boolean. false))
false
user=> (not (Boolean. true))
false
user=> (not (Boolean. "true"))
false
user=> (not (Boolean/valueOf "false"))
true



 Comments   
Comment by Kuldeep [ 13/Jan/15 3:55 AM ]

http://clojure.org/special_forms#Special Forms--(if test then else?)





[CLJ-1639] Loading both AOT and non-AOT versions of a namespace causes errors Created: 12/Jan/15  Updated: 29/Jan/15  Resolved: 16/Jan/15

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

Type: Defect Priority: Critical
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: regression
Environment:

Clojure 1.7.0-alpha5



 Description   

Change in behavior here due to CLJ-979 added in 1.7.0-alpha5.

Symptom is that code fails to compile when a namespace is loading, claiming a protocol has no implementation for a method (that it does have).

Stack trace from Sean Corfield's code:

Caused by: java.lang.IllegalArgumentException: No implementation of method: :has? of protocol: #'clojure.core.cache/CacheProtocol found for class: clojure.core.memoize.PluggableMemoization 
        at clojure.core$_cache_protocol_fn.invoke(core_deftype.clj:555) 
        at clojure.core.cache$eval1710$fn__1773$G__1699__1780.invoke(cache.clj:20) 
        at clojure.core.cache$through.invoke(cache.clj:53) 
        at clojure.core.memoize$through_STAR_.invoke(memoize.clj:52) 
        at clojure.lang.Atom.swap(Atom.java:65) 
        at clojure.core$swap_BANG_.invoke(core.clj:2236) 
        at clojure.core.memoize$build_memoizer$fn__12665.doInvoke(memoize.clj:134) 
        at clojure.lang.RestFn.applyTo(RestFn.java:137) 
        at clojure.lang.AFunction$1.doInvoke(AFunction.java:29) 
        at clojure.lang.RestFn.invoke(RestFn.java:408) 
        at clojure.tools.analyzer.jvm$desugar_host_expr.invoke(jvm.clj:117) 
        at clojure.tools.analyzer.jvm$macroexpand_1.invoke(jvm.clj:174) 
        at clojure.tools.analyzer$fn__12389.invoke(analyzer.clj:281) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:238) 
        at clojure.tools.analyzer$fn__12346.invoke(analyzer.clj:68) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:233) 
        at clojure.tools.analyzer$analyze_let.invoke(analyzer.clj:505) 
        at clojure.tools.analyzer$fn__12469.invoke(analyzer.clj:530) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:233) 
        at clojure.tools.analyzer.jvm$fn__13956.invoke(jvm.clj:66) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:233) 
        at clojure.tools.analyzer$fn__12389.invoke(analyzer.clj:283) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:238) 
        at clojure.tools.analyzer$fn__12346.invoke(analyzer.clj:68) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:233) 
        at clojure.tools.analyzer$fn__12389.invoke(analyzer.clj:284) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:238) 
        at clojure.tools.analyzer$fn__12346.invoke(analyzer.clj:68) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:233) 
        at clojure.tools.analyzer$fn__12392.invoke(analyzer.clj:295) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:233) 
        at clojure.tools.analyzer.jvm$fn__13956.invoke(jvm.clj:66) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:233) 
        at clojure.tools.analyzer$analyze_body.invoke(analyzer.clj:366) 
        at clojure.tools.analyzer$analyze_let.invoke(analyzer.clj:520) 
        at clojure.tools.analyzer$fn__12471.invoke(analyzer.clj:540) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:233) 
        at clojure.tools.analyzer.jvm$fn__13956.invoke(jvm.clj:66) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:233) 
        at clojure.tools.analyzer$fn__12389.invoke(analyzer.clj:283) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:238) 
        at clojure.tools.analyzer$fn__12346.invoke(analyzer.clj:68) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:233) 
        at clojure.tools.analyzer$fn__12389.invoke(analyzer.clj:284) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:238) 
        at clojure.tools.analyzer$fn__12346.invoke(analyzer.clj:68) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:233) 
        at clojure.tools.analyzer$fn__12392.invoke(analyzer.clj:295) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:233) 
        at clojure.tools.analyzer.jvm$fn__13956.invoke(jvm.clj:66) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:233) 
        at clojure.tools.analyzer$fn__12389.invoke(analyzer.clj:283) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:238) 
        at clojure.tools.analyzer$fn__12346.invoke(analyzer.clj:68) 
        at clojure.lang.MultiFn.invoke(MultiFn.java:233) 
        at clojure.tools.analyzer$analyze.invoke(analyzer.clj:123) 
        at clojure.tools.analyzer.jvm$analyze$fn__14085.invoke(jvm.clj:476) 
        at clojure.lang.AFn.applyToHelper(AFn.java:152) 
        at clojure.lang.AFn.applyTo(AFn.java:144) 
        at clojure.core$apply.invoke(core.clj:626) 
        at clojure.core$with_bindings_STAR_.doInvoke(core.clj:1864) 
        at clojure.lang.RestFn.invoke(RestFn.java:425) 
        at clojure.tools.analyzer.jvm$analyze.invoke(jvm.clj:474) 
        at clojure.tools.analyzer.jvm$analyze.invoke(jvm.clj:467) 
        at clojure.core.async.impl.ioc_macros$state_machine.invoke(ioc_macros.clj:1062) 
        at clojure.core.async$go.doInvoke(async.clj:384) 
        at clojure.lang.RestFn.invoke(RestFn.java:442) 
        at clojure.lang.Var.invoke(Var.java:388) 
        at clojure.lang.AFn.applyToHelper(AFn.java:160) 
        at clojure.lang.Var.applyTo(Var.java:700) 
        at clojure.lang.Compiler.macroexpand1(Compiler.java:6606)

Stacktrace from Andy Fingerhut's code, in Eastwood:

% lein do clean, eastwood '{:namespaces [clojure.reflect]}'
== Eastwood 0.2.1 Clojure 1.7.0-alpha4c979only JVM 1.7.0_45
== Linting clojure.reflect ==
Exception thrown during phase :analyze+eval of linting namespace clojure.reflect
IllegalArgumentException No implementation of method: :do-reflect of protocol: #'clojure.reflect/Reflector found for class: clojure.reflect.JavaReflector
	clojure.core/-cache-protocol-fn (core_deftype.clj:555)
	clojure.reflect/eval6486/fn--6487/G--6470--6490 (form-init6080826551765301071.clj:1)
	clojure.core/partial/fn--4490 (core.clj:2489)
	clojure.reflect/type-reflect (reflect.clj:100)
	clojure.core/apply (core.clj:632)
	eastwood.copieddeps.dep2.clojure.tools.analyzer.jvm.utils/type-reflect (utils.clj:24)
	eastwood.copieddeps.dep2.clojure.tools.analyzer.jvm.utils/members*--1293 (utils.clj:271)
	clojure.core/apply (core.clj:626)
	eastwood.copieddeps.dep3.clojure.core.memoize/through*/fn--1072 (memoize.clj:66)
	eastwood.copieddeps.dep4.clojure.core.cache/through/fn--872 (cache.clj:55)
	eastwood.copieddeps.dep3.clojure.core.memoize/through*/fn--1068/fn--1069 (memoize.clj:65)
	eastwood.copieddeps.dep3.clojure.core.memoize/d-lay/reify--1063 (memoize.clj:54)
	clojure.core/deref (core.clj:2202)
	eastwood.copieddeps.dep3.clojure.core.memoize/build-memoizer/fn--1123 (memoize.clj:152)
	clojure.lang.AFunction$1.doInvoke (AFunction.java:29)
	eastwood.copieddeps.dep2.clojure.tools.analyzer.jvm.utils/members (utils.clj:280)
	eastwood.copieddeps.dep2.clojure.tools.analyzer.jvm.utils/instance-members (utils.clj:289)
	eastwood.copieddeps.dep2.clojure.tools.analyzer.jvm.utils/instance-methods (utils.clj:299)
	eastwood.copieddeps.dep2.clojure.tools.analyzer.passes.jvm.validate/validate-call (validate.clj:91)
	eastwood.copieddeps.dep2.clojure.tools.analyzer.passes.jvm.validate/eval2056/fn--2058 (validate.clj:148)
	clojure.lang.MultiFn.invoke (MultiFn.java:229)
	eastwood.copieddeps.dep2.clojure.tools.analyzer.passes.jvm.validate/validate (validate.clj:264)
	clojure.lang.Var.invoke (Var.java:379)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.passes/compile-passes/fn--574/fn--579 (passes.clj:171)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.passes/compile-passes/fn--574/fn--581 (passes.clj:173)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.passes/compile-passes/fn--574/fn--581 (passes.clj:173)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.passes/compile-passes/fn--574/fn--581 (passes.clj:173)
	clojure.core/partial/fn--4492 (core.clj:2496)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191 (ast.clj:102)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191/walk--192 (ast.clj:96)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children/fn--182 (ast.clj:51)
	clojure.lang.ArrayChunk.reduce (ArrayChunk.java:63)
	clojure.core.protocols/fn--6433 (protocols.clj:103)
	clojure.core.protocols/fn--6395/G--6390--6404 (protocols.clj:19)
	clojure.core.protocols/seq-reduce (protocols.clj:31)
	clojure.core.protocols/fn--6414 (protocols.clj:65)
	clojure.core.protocols/fn--6369/G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6461)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children (ast.clj:56)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/update-children-reduced (ast.clj:64)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191 (ast.clj:99)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191/walk--192 (ast.clj:96)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children/fn--182 (ast.clj:51)
	clojure.lang.ArrayChunk.reduce (ArrayChunk.java:63)
	clojure.core.protocols/fn--6433 (protocols.clj:103)
	clojure.core.protocols/fn--6395/G--6390--6404 (protocols.clj:19)
	clojure.core.protocols/seq-reduce (protocols.clj:31)
	clojure.core.protocols/fn--6414 (protocols.clj:65)
	clojure.core.protocols/fn--6369/G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6461)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children (ast.clj:56)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/update-children-reduced (ast.clj:64)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191 (ast.clj:99)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191/walk--192 (ast.clj:96)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.utils/mapv' (utils.clj:208)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children/fn--182 (ast.clj:51)
	clojure.lang.ArrayChunk.reduce (ArrayChunk.java:58)
	clojure.core.protocols/fn--6433 (protocols.clj:103)
	clojure.core.protocols/fn--6395/G--6390--6404 (protocols.clj:19)
	clojure.core.protocols/seq-reduce (protocols.clj:31)
	clojure.core.protocols/fn--6414 (protocols.clj:65)
	clojure.core.protocols/fn--6369/G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6461)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children (ast.clj:56)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/update-children-reduced (ast.clj:64)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191 (ast.clj:99)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191/walk--192 (ast.clj:96)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children/fn--182 (ast.clj:51)
	clojure.lang.ArrayChunk.reduce (ArrayChunk.java:58)
	clojure.core.protocols/fn--6433 (protocols.clj:103)
	clojure.core.protocols/fn--6395/G--6390--6404 (protocols.clj:19)
	clojure.core.protocols/seq-reduce (protocols.clj:31)
	clojure.core.protocols/fn--6414 (protocols.clj:65)
	clojure.core.protocols/fn--6369/G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6461)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children (ast.clj:56)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/update-children-reduced (ast.clj:64)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191 (ast.clj:99)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191/walk--192 (ast.clj:96)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.utils/mapv' (utils.clj:208)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children/fn--182 (ast.clj:51)
	clojure.lang.ArrayChunk.reduce (ArrayChunk.java:58)
	clojure.core.protocols/fn--6433 (protocols.clj:103)
	clojure.core.protocols/fn--6395/G--6390--6404 (protocols.clj:19)
	clojure.core.protocols/seq-reduce (protocols.clj:31)
	clojure.core.protocols/fn--6414 (protocols.clj:65)
	clojure.core.protocols/fn--6369/G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6461)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children (ast.clj:56)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/update-children-reduced (ast.clj:64)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191 (ast.clj:99)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191/walk--192 (ast.clj:96)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children/fn--182 (ast.clj:51)
	clojure.lang.ArrayChunk.reduce (ArrayChunk.java:63)
	clojure.core.protocols/fn--6433 (protocols.clj:103)
	clojure.core.protocols/fn--6395/G--6390--6404 (protocols.clj:19)
	clojure.core.protocols/seq-reduce (protocols.clj:31)
	clojure.core.protocols/fn--6414 (protocols.clj:65)
	clojure.core.protocols/fn--6369/G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6461)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children (ast.clj:56)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/update-children-reduced (ast.clj:64)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191 (ast.clj:99)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191/walk--192 (ast.clj:96)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children/fn--182 (ast.clj:51)
	clojure.lang.ArrayChunk.reduce (ArrayChunk.java:63)
	clojure.core.protocols/fn--6433 (protocols.clj:103)
	clojure.core.protocols/fn--6395/G--6390--6404 (protocols.clj:19)
	clojure.core.protocols/seq-reduce (protocols.clj:31)
	clojure.core.protocols/fn--6414 (protocols.clj:65)
	clojure.core.protocols/fn--6369/G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6461)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children (ast.clj:56)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/update-children-reduced (ast.clj:64)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191 (ast.clj:99)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191/walk--192 (ast.clj:96)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.utils/mapv' (utils.clj:208)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children/fn--182 (ast.clj:51)
	clojure.lang.ArrayChunk.reduce (ArrayChunk.java:58)
	clojure.core.protocols/fn--6433 (protocols.clj:103)
	clojure.core.protocols/fn--6395/G--6390--6404 (protocols.clj:19)
	clojure.core.protocols/seq-reduce (protocols.clj:31)
	clojure.core.protocols/fn--6414 (protocols.clj:65)
	clojure.core.protocols/fn--6369/G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6461)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children (ast.clj:56)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/update-children-reduced (ast.clj:64)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191 (ast.clj:99)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191/walk--192 (ast.clj:96)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.utils/mapv' (utils.clj:208)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children/fn--182 (ast.clj:51)
	clojure.lang.ArrayChunk.reduce (ArrayChunk.java:63)
	clojure.core.protocols/fn--6433 (protocols.clj:103)
	clojure.core.protocols/fn--6395/G--6390--6404 (protocols.clj:19)
	clojure.core.protocols/seq-reduce (protocols.clj:31)
	clojure.core.protocols/fn--6414 (protocols.clj:65)
	clojure.core.protocols/fn--6369/G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6461)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children (ast.clj:56)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/update-children-reduced (ast.clj:64)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191 (ast.clj:99)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191/walk--192 (ast.clj:96)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.utils/mapv' (utils.clj:208)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children/fn--182 (ast.clj:51)
	clojure.lang.ArrayChunk.reduce (ArrayChunk.java:63)
	clojure.core.protocols/fn--6433 (protocols.clj:103)
	clojure.core.protocols/fn--6395/G--6390--6404 (protocols.clj:19)
	clojure.core.protocols/seq-reduce (protocols.clj:31)
	clojure.core.protocols/fn--6414 (protocols.clj:65)
	clojure.core.protocols/fn--6369/G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6461)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children (ast.clj:56)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/update-children-reduced (ast.clj:64)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191 (ast.clj:99)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191/walk--192 (ast.clj:96)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.utils/mapv' (utils.clj:208)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children/fn--182 (ast.clj:51)
	clojure.lang.ArrayChunk.reduce (ArrayChunk.java:63)
	clojure.core.protocols/fn--6433 (protocols.clj:103)
	clojure.core.protocols/fn--6395/G--6390--6404 (protocols.clj:19)
	clojure.core.protocols/seq-reduce (protocols.clj:31)
	clojure.core.protocols/fn--6414 (protocols.clj:65)
	clojure.core.protocols/fn--6369/G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6461)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/-update-children (ast.clj:56)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/update-children-reduced (ast.clj:64)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk/walk--191 (ast.clj:99)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/walk (ast.clj:95)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/postwalk (ast.clj:115)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.ast/postwalk (ast.clj:113)
	eastwood.copieddeps.dep1.clojure.tools.analyzer.passes/compile-passes/analyze--586 (passes.clj:175)
	clojure.core/comp/fn--4458 (core.clj:2434)
	clojure.core/comp/fn--4458 (core.clj:2434)
	eastwood.analyze-ns/run-passes (analyze_ns.clj:157)
	eastwood.copieddeps.dep2.clojure.tools.analyzer.jvm/analyze/fn--3553 (jvm.clj:474)
	clojure.core/apply (core.clj:626)
	clojure.core/with-bindings* (core.clj:1864)
	eastwood.copieddeps.dep2.clojure.tools.analyzer.jvm/analyze (jvm.clj:470)
	eastwood.copieddeps.dep2.clojure.tools.analyzer.jvm/analyze+eval (jvm.clj:518)
	eastwood.copieddeps.dep2.clojure.tools.analyzer.jvm/analyze+eval/fn--3574 (jvm.clj:505)
	clojure.core/mapv/fn--6657 (core.clj:6558)
	clojure.lang.ArrayChunk.reduce (ArrayChunk.java:63)
	clojure.core.protocols/fn--6433 (protocols.clj:103)
	clojure.core.protocols/fn--6395/G--6390--6404 (protocols.clj:19)
	clojure.core.protocols/seq-reduce (protocols.clj:31)
	clojure.core.protocols/fn--6418 (protocols.clj:53)
	clojure.core.protocols/fn--6369/G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6461)
	clojure.core/mapv (core.clj:6558)
	eastwood.copieddeps.dep2.clojure.tools.analyzer.jvm/analyze+eval (jvm.clj:503)
	eastwood.analyze-ns/analyze-file/fn--4173/fn--4175 (analyze_ns.clj:279)
	eastwood.analyze-ns/analyze-file/fn--4173 (analyze_ns.clj:276)
	eastwood.analyze-ns/analyze-file (analyze_ns.clj:274)
	eastwood.analyze-ns/analyze-ns (analyze_ns.clj:327)
	eastwood.lint/lint-ns (lint.clj:569)
	eastwood.lint/eastwood-core/fn--6336 (lint.clj:1041)
	eastwood.lint/eastwood-core (lint.clj:1040)
	eastwood.lint/eastwood (lint.clj:1154)
	eastwood.lint/eastwood-from-cmdline (lint.clj:1167)
	clojure.lang.Var.invoke (Var.java:379)
	eastwood.versioncheck/run-eastwood (versioncheck.clj:15)
	user/eval21 (form-init6080826551765301071.clj:1)
	clojure.lang.Compiler.eval (Compiler.java:6767)
	clojure.lang.Compiler.eval (Compiler.java:6757)
	clojure.lang.Compiler.load (Compiler.java:7194)
	clojure.lang.Compiler.loadFile (Compiler.java:7150)
	clojure.main/load-script (main.clj:274)
	clojure.main/init-opt (main.clj:279)
	clojure.main/initialize (main.clj:307)
	clojure.main/null-opt (main.clj:342)
	clojure.main/main (main.clj:420)
	clojure.lang.Var.invoke (Var.java:383)
	clojure.lang.Var.applyTo (Var.java:700)
	clojure.main.main (main.java:37)

The following form was being processed during the exception:

(defprotocol
 TypeReference
 "A TypeReference can be unambiguously converted to a type name on\n   the host platform.\n\n   All typerefs are normalized into symbols. If you need to\n   normalize a typeref yourself, call typesym."
 (typename
  [o]
  "Returns Java name as returned by ASM getClassName, e.g. byte[], java.lang.String[]"))

Shown again with metadata for debugging (some metadata elided for brevity):

^{:line 48}
(^{:line 48} defprotocol
 ^{:line 48} TypeReference
 "A TypeReference can be unambiguously converted to a type name on\n   the host platform.\n\n   All typerefs are normalized into symbols. If you need to\n   normalize a typeref yourself, call typesym."
 ^{:line 54}
 (^{:line 54} typename
  ^{:line 54} [^{:line 54} o]
  "Returns Java name as returned by ASM getClassName, e.g. byte[], java.lang.String[]"))

An exception was thrown while analyzing namespace clojure.reflect 
Lint results may be incomplete.  If there are compilation errors in
your code, try fixing those.  If not, check above for info on the
exception.

Exception thrown while analyzing last namespace.

== Warnings: 0 (not including reflection warnings)  Exceptions thrown: 1
Subprocess failed


 Comments   
Comment by Nicola Mometto [ 12/Jan/15 2:04 PM ]

I can't tell about the exception Sean is getting, but the one Andy is getting with eastwood, I believe should not be considered a bug.

What's happening in eastwood is that the entire clojure.reflect namespace is being reloaded, deftypes and defprotocols included, while a reference to an instance of a previous deftype is being used by the eastwood codebase (from the tools.analyzer.jvm dep)

This used to work only because of the bug that CLJ-979 fixed where since clojure.reflect is AOT compiled, re-evaluating the defprotocol didn't change the underlying interface.

This code demonstrates this:

;; PRE CLJ-979, re-evaluating an AOT compiled defprotocol didn't change the underlying interface used
user=> *clojure-version*
{:interim true, :major 1, :minor 7, :incremental 0, :qualifier "alpha4"}
user=> (use 'clojure.reflect)
nil
user=> (in-ns 'clojure.reflect)
#<Namespace clojure.reflect>
clojure.reflect=> (hash (:on-interface Reflector))
2141314409
clojure.reflect=> (defprotocol Reflector (do-reflect [reflector typeref]))
Reflector
clojure.reflect=> (hash (:on-interface Reflector))
2141314409
clojure.reflect=>


;; AFTER CLJ-979, re-evaluating an AOT compiled defprotocol _does_ change the underlying interface used
user=> *clojure-version*
{:interim true, :major 1, :minor 7, :incremental 0, :qualifier "master"}
user=> (use 'clojure.reflect)
nil
user=> (in-ns 'clojure.reflect)
#<Namespace clojure.reflect>
clojure.reflect=> (hash (:on-interface Reflector))
390902174
clojure.reflect=> (defprotocol Reflector (do-reflect [reflector typeref]))
Reflector
clojure.reflect=> (hash (:on-interface Reflector))
1673776464


;; note that while the new behaviour causes the eastwood bug (and maybe the one Sean is seeing too),
;; the new behaviour is consistent with how redefining a protocol not AOT compiled behaved:
user=> *clojure-version*
{:interim true, :major 1, :minor 7, :incremental 0, :qualifier "alpha4"}
user=> (defprotocol foo)
foo
user=> (hash (:on-interface foo))
1058055630
user=> (defprotocol foo)
foo
user=> (hash (:on-interface foo))
1333687570

Again, I don't know if the exception that Sean is getting is related to this issue without looking at the code, bug I suspect a similar scenario given that Sean told me in IRC that there is indeed some AOT compilation involved with a wrapper around clojure.cache.

I personally wouldn't consider this a bug, I want to emphatize that the exception demonstrated by Andy would have occurred in 1.7.0-alpha4 too hadn't clojure.reflect been AOT compiled and the fact that it worked instead is just an accident of the bug fixed with CLJ-979.

Comment by Sean Corfield [ 12/Jan/15 2:19 PM ]

The only AOT compilation in our entire code base is one small library that implements a DBAppender for log4j. That's a separate project that we AOT compile and then depend on in our main (non-AOT) projects. That AOT project, in order to avoid pulling in a lot of transitive dependencies that get AOT-compiled too (due to the over-enthusiasm of Clojure's AOT process), uses runtime require/resolve to load the symbols it needs from the main project.

Given what you're saying, it sounds like that runtime require/resolve is broken by fixing CLJ-979 (and was therefore only working "by accident" before)?

Comment by Nicola Mometto [ 12/Jan/15 2:34 PM ]

Here a better test case showcasing how 1.7.0-alpha5 behaves consistently between AOT and JIT scenarios while 1.7.0-alpha4 has a different behaviour when AOT compilation is involved. 1.7.0-alpha4 JIT is consistent with 1.7.0-alpha5 JIT and AOT

[~]> cat test.clj
(ns test)
(defprotocol p (f [_]))
(deftype t [] p (f [_] 1))
[~]> mkdir classes
;; 1.7.0-alpha4 AOT
[~]> java -cp .m2/repository/org/clojure/clojure/1.7.0-alpha4/clojure-1.7.0-alpha4.jar:.:classes clojure.main
Clojure 1.7.0-alpha4
user=> (binding [*compile-files* true] (load "test"))
nil
user=> (in-ns 'test)
#<Namespace test>
test=> (def a (t.))
#'test/a
test=> (defprotocol p (f [_]))
p
test=> (deftype t [] p (f [_] 1))
test.t
test=> (f a)
1
;; notice how this is the only call that succeds since the re-defined protocol is still backed by the 
;; AOT compiled class rather than the one generated JIT with the defprotocol call at the repl
test=>
[~]> rm -rf classes/*
;; 1.7.0-alpha4 no AOT
[~]> java -cp .m2/repository/org/clojure/clojure/1.7.0-alpha4/clojure-1.7.0-alpha4.jar:.:classes clojure.main
Clojure 1.7.0-alpha4
user=> (load "test")
nil
user=> (in-ns 'test)
#<Namespace test>
test=> (def a (t.))
#'test/a
test=> (defprotocol p (f [_]))
p
test=> (deftype t [] p (f [_] 1))
test.t
test=> (f a)
IllegalArgumentException No implementation of method: :f of protocol: #'test/p found for class: test.t  clojure.core/-cache-protocol-fn (core_deftype.clj:555)
test=>
;; 1.7.0-alpha5 AOT
[~]> java -cp .m2/repository/org/clojure/clojure/1.7.0-alpha5/clojure-1.7.0-alpha5.jar:.:classes clojure.main
Clojure 1.7.0-alpha5
user=> (binding [*compile-files* true] (load "test"))
nil
user=> (in-ns 'test)
#<Namespace test>
test=> (def a (t.))
#'test/a
test=> (defprotocol p (f [_]))
p
test=> (deftype t [] p (f [_] 1))
test.t
test=> (f a)
IllegalArgumentException No implementation of method: :f of protocol: #'test/p found for class: test.t  clojure.core/-cache-protocol-fn (core_deftype.clj:555)
test=>
[~]> rm -rf classes/*
;; 1.7.0-alpha5 no AOT
[~]> java -cp .m2/repository/org/clojure/clojure/1.7.0-alpha5/clojure-1.7.0-alpha5.jar:.:classes clojure.main
Clojure 1.7.0-alpha5
user=> (load "test")
nil
user=> (in-ns 'test)
#<Namespace test>
test=> (def a (t.))
#'test/a
test=> (defprotocol p (f [_]))
p
test=> (deftype t [] p (f [_] 1))
test.t
test=> (f a)
IllegalArgumentException No implementation of method: :f of protocol: #'test/p found for class: test.t  clojure.core/-cache-protocol-fn (core_deftype.clj:555)
test=>
Comment by Sean Corfield [ 12/Jan/15 2:37 PM ]

I removed the runtime require in the AOT'd project and we still get this exception so I'm less inclined to believe our usage is buggy here. The only other runtime require we do is on our New Relic wrapper - but we get the exception in a pure Clojure project that does not use that at all (and now has no runtime require calls at all, as far as I can tell).

Comment by Nicola Mometto [ 12/Jan/15 2:40 PM ]

Sean, I cannot be sure about your case as I don't know the code/compilation scenario behind the exception.
All I said is however true for the exception reported by Andy and that the two exceptions apperas to be the same and to be caused by the same issue.

I can't also tell you whether this will be considered a regression or if the it will be chosen that your code happened to work by accident before.

I personally don't believe this should be considered a regression since the new behaviour makes JIT and AOT consistent while previously they weren't, but I don't have the authority to decide on this matter

Comment by Sean Corfield [ 12/Jan/15 3:39 PM ]

To rule out some other interactions in our code, I moved our one and only AOT-compiled file/namespace into the main project and removed the dependency on that separate library, and I also made sure there are no require or other dynamic loading occurring at runtime. The AOT-compiled namespace has no dependencies except on clojure.core and I verified that no .class files are generated for anything except that single namespace.

I still get that exception, always on `PluggableMemoization`. I'm going to start going through the libraries we depend on and see if anything else might be bringing in an AOT-version of either core.cache or core.memoize.

Comment by Sean Corfield [ 12/Jan/15 3:47 PM ]

Searching through the libraries we use, core.typed seems to contain AOT'd versions of core.cache and core.memoize so I'm going to build a version of our system without core.typed to see if that's the culprit here.

Comment by Sean Corfield [ 12/Jan/15 4:32 PM ]

Removing core.typed completely from our system seems to resolve the problem. I'm still dealing with the fallout from other, earlier changes made for debugging but at this point I'm fairly confident that without core.typed, our applications will run on Alpha 5. Will update again when I'm done testing.

Comment by Sean Corfield [ 12/Jan/15 5:31 PM ]

Confirmed: removing core.typed allows all of our tests to pass and all of our applications to work correctly on Alpha 5. I'll raised an issue against core.typed at this point. If Clojure/core feel this ticket is not a bug, it can be closed.

Comment by Nicola Mometto [ 13/Jan/15 10:14 AM ]

Sean, for the scenario to be what I described is happening in eastwood, loading an AOT core.[cache/memoize] is not enough.
What needs to go on is also having core.[cache/memoize] realoaded JIT after the AOT compiled version has been loaded, this can happen if for example the version that core.typed ships with is older than the one one of your deps is pulling in.

And again, I personally think that this scenario should not be considered a bug but I don't speak for Clojure/core so it's possible that Alex, Rich or some other screener will have a different opinion than mine.

Comment by Sean Corfield [ 13/Jan/15 12:44 PM ]

Sorry if it wasn't clear from my stream of comments: our apps rely on core.cache and so it is brought in via JIT in some namespaces - as well as the AOT version loaded via core.typed in other namespaces. So, yes, the scenario you describe is definitely happening in our code - it just took me a while to find where the AOT version was coming from.

I consider this a bug against core.typed - it should not ship AOT versions of other libraries that folks might also be using via JIT - and created an issue in JIRA for that.

This fix to CLJ-979 will mean that no library would ever be able to bundle AOT versions of other libraries (that contained classes generated via deftype etc) since any applications that used said library - and also used any of those other libraries - would trip over this. That may well be a good thing: bundling libraries can already cause version conflicts and mixing AOT in just makes that harder to debug and harder to deal with.

Comment by Alex Miller [ 15/Jan/15 2:40 PM ]

I agree that shipping versions of other libraries inside your own jar (as core.typed appears to be doing) is bad.

I am a little concerned that from a user's POV maybe we have just replaced one "AOT is weird and doesn't work like I expect" with another, regardless of how consistent that behavior is.

Still something I'm just mulling through.

Comment by Nicola Mometto [ 15/Jan/15 2:53 PM ]

Alex, I share your worry however I'd like to point out that if the cause of the error Sean is getting is the same as the cause for the exception for eastwood, that is, the reloading of namespaces containg deftypes/defrecord after one instance of a said type/interface has been used, then we should expect things to break as there is now no difference between how that behaves under JIT and under AOT.

In other words, I don't believe it is reasonable to expect code like this to work:

(deftype X [a])
(def a (X. 1))
(deftype X [a])
(.a ^X a)

The fact that in some scenarios (strictly under AOT!) code similar to that used to work rather than throw should not be a reason to consider the new, consistent behaviour, wrong IMHO.

Note that if it turns out that Sean's exception (or any other other reported issue caused by CLJ-979) is caused by a different scenario than the one I described, then I agree that this is a regression that should be addressed

Comment by Alex Miller [ 16/Jan/15 12:56 PM ]

No plans for further action on this ticket and will pursue a fix for the bad core.typed jar under that ticket.

Comment by Sean Corfield [ 16/Jan/15 1:11 PM ]

I agree no further action is needed but have a question / suggestion:

Is there a reasonable way for Clojure to detect the situation and provide a better error message?

Or is it perhaps worth expanding the error message to include a suggestion to check whether a namespace has been reloaded or a type has been redefined?

Comment by Alex Miller [ 16/Jan/15 3:15 PM ]

It's a good question. I'm not sure that it's easy to detect?

Comment by Nicola Mometto [ 29/Jan/15 1:29 PM ]

It's possible that CLJ-1650 is actually what's causing Sean's exception.





[CLJ-1638] Regression - PersistentVector.create(List) was removed in 1.7.0-alpha5 Created: 12/Jan/15  Updated: 27/Feb/15

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

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, regression
Environment:

1.7.0-alpha5


Attachments: Text File clj-1638-2.patch     Text File clj-1638.patch    
Patch: Code
Approval: Screened

 Description   

For CLJ-1546, PersistentVector.create(List) was replaced with PersistentVector.create(ArrayList). At least one library (flatland) was calling this method directly and was broken by the change.

Approach: Change create(ArrayList) to more general prior method create(List).

Patch: clj-1638-2.patch

Screened by:



 Comments   
Comment by Rich Hickey [ 20/Feb/15 7:42 AM ]

Is there a good reason to have both PersistentVector.create(List)and PersistentVector.create(ArrayList)?

Comment by Fogus [ 27/Feb/15 9:08 AM ]

This couldn't possibly be more straight-forward.





[CLJ-1637] vec fails on MapEntry Created: 11/Jan/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: regression
Environment:

1.7.0-alpha5


Attachments: Text File clj-1637-jdevuyst.patch     Text File clj-1637.patch     Text File clj-1637-with-test.patch    
Patch: Code
Approval: Ok

 Description   

After CLJ-1546:

(vec (first {1 2}))

Cause: (if (vector? coll) (with-meta coll nil) ...) checks that something is IPersistentVector, then sends it to something that takes IObj, so anything that is one but not both throws an error. In Clojure itself, this is the set of classes extending from AMapEntry.

Alternatives:

1. Make AMapEntry implement IObj - this fixes everything in Clojure and keeps the vec code as is but still leaves open this gap for any external implementation of IPersistentVector.
2. Check for this case explicitly in vec. (if (vector? coll) (if (instance? clojure.lang.IObj coll) (with-meta coll nil) (clojure.lang.LazilyPersistentVector/create coll)) ...). Perf testing shows no significant difference in performance with the change.
3. Pull the special check for vector? in vec.
4. Check for this case explicitly in vec and return the same instance if it's not an IObj. See clj-1637-jdevuyst.patch.

Approach: patch takes approach #2

Patch: clj-1637-with-test.patch

Screened by: Stu (also added test)



 Comments   
Comment by Nicola Mometto [ 11/Jan/15 8:19 AM ]

The correct fix is to probably make MapEntry an IObj

Comment by Nicola Mometto [ 11/Jan/15 8:21 AM ]

Actually, making AMapEntry an IObj rather than MapEntry would fix the issue for sorted-map kv-pairs too.

user=> (vec (first (sorted-map 1 1)))
ClassCastException clojure.lang.PersistentTreeMap$BlackVal cannot be cast to clojure.lang.IObj  clojure.core/with-meta--4121 (core.clj:216)
Comment by Alex Miller [ 11/Jan/15 8:35 AM ]

There are potentially a couple ways to fix this. I'll look at it next week.

Comment by Jonas De Vuyst [ 14/Jan/15 6:14 AM ]

Slightly modified patch. In the case where coll is a vector but not an IObj, simply return coll.

Comment by Jonas De Vuyst [ 14/Jan/15 7:17 AM ]

If desired I could also update the patch to fix an analogous—albeit somewhat theoretic—bug in `set`.

It does make me wonder if perhaps a `without-meta` function should be added to `clojure.core`.

I think making AMapEntry an IObj might make sense even after applying the above patches. In `AMapEntry`, perhaps `withMeta(m)` could be implemented as `asVector().withMeta(m)`. This, however, would require changing `asVector()` to return some `IObj ∩ IPersistentVector` type (e.g. `PersistentVector`). This would be straightforward to do, but requires deciding if this change in signature may be propagated to the static methods of `LazilyPersistentVector`.

Comment by Alex Miller [ 14/Jan/15 9:28 AM ]

This is a point of some debate, but the intention with the change in the implementation is to retain current behavior, which always gives you a new vector instance. It's not clear to me that there is any point in attaching meta to map entries (which also does not solve the problem for external IPersistentVector, non-IObj instances outside Clojure).

In any case, I'm going to update the description a bit to add this as an alternative.





[CLJ-1636] SeqIterator can return incorrect results Created: 10/Jan/15  Updated: 18/Jan/15  Resolved: 16/Jan/15

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

Type: Defect Priority: Blocker
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: regression
Environment:

clojure-1.7.0-alpha5


Attachments: Text File 0001-CLJ-1636-don-t-use-this-as-a-sentinel-in-SeqIterator.patch     Text File 0001-fix-for-CLJ-1636.patch    
Patch: Code
Approval: Ok

 Description   

As of 1.7.0-alpha5, we are seeing SeqIterator return iterated results that do reflect the values of the underlying seq, in particular acting as if the seq contains a nil value when it does not. This problem is intermittent but has at times caused clojure master to fail in compilation (which is why this is marked as a blocker).

Two recent changes during 1.7 have created and exposed this problem:

1) This commit https://github.com/clojure/clojure/commit/43cc1854508d655e58e377f84836ba128971f90c changed the SeqIterator implementation to be lazier and to use "this" as a sentinel object in SeqIterator. (1.7.0-alpha2)
2) CLJ-1546 changed the implementation of vec such that PersistentHashMap and PersistentHashSet are now converted using iterator() rather than seq(). PHS/PHM use SeqIterator for their Iterator implementation. (1.7.0-alpha5)

Because of #2, we are now stressing #1 much more than before. In particular, things like defining defrecords rely heavily on vec (and set) of PHS and PHM.

Example stack trace: https://gist.github.com/puredanger/f56e3253f0668a515ec5 (seen compiling Clojure itself)

Cause: Setting seq==this; in the constructor of SeqIterator is allowing unsafe publication of the partially constructed "this" object, which can cause subtle problems in the hasNext() implementation. In particular, it seems that after inlining, on the first call, the seq==this condition when comparing the cached partially constructed instance in seq and the fully constructed version in this will return false, even though these have the same object identity. This causes the wrong path to be executed in hasNext().

Approach: Do not use this as a sentinel value.

Patch: 0001-CLJ-1636-don-t-use-this-as-a-sentinel-in-SeqIterator.patch

Screened by: Alex Miller



 Comments   
Comment by Colin Jones [ 11/Jan/15 12:40 AM ]

I was able to reproduce this (intermittently) earlier, but I've seen periods of many successful runs in a row (both with that patch reverted and with it in place), so it's been hard for me to trust what I'm seeing locally when it passes. I didn't see any evidence of AOT compilation happening (e.g. no classfiles under `target/`), so I'd have expected the new function `already-compiled?` in CLJ-1544 never to actually run.

It looks like the Cause section of the stacktrace is implicating an error in trying to `(resolve nil)`, where `nil` is an entry in an interfaces collection that should actually be empty. That's based on these two lines (along with the lines higher up in the cause):

...snip...
at clojure.core$set.invoke(core.clj:3944)
at clojure.core$emit_defrecord.invoke(core_deftype.clj:154)
...snip...

(https://github.com/clojure/clojure/blob/3e7cb1a5c840612ad41cf6e0be92480f798bc05d/src/clj/clojure/core_deftype.clj#L154)

The defrecord here in question looks like

(defrecord Foo [x y])

So the `opts+specs` var-arg argument to `defrecord` should be `nil` since there are no entries, which should mean the `interfaces` piece of the `parse-opts+specs` call should return an empty vector.

But that stacktrace confuses me, because it suggests that the `interfaces` vector, instead of being empty, contains a `nil` element. How can this be? Or what misstep have I made in tracing through this?

Comment by Alex Miller [ 11/Jan/15 12:54 AM ]

If the error is intermittent, then my pegging of CLJ-1544 may be wrong. For me, it was repeatable as of clojure commit e5a104e894ed82f244d69513918d570cee5df67d (when CLJ-1544 was applied) and I have not reproduced it prior.

Comment by Nicola Mometto [ 11/Jan/15 4:50 AM ]

Alex, just to be sure – you were able to reproduce this bug with clojure at e5a104e894ed82f244d69513918d570cee5df67d (CLJ-1544) ? I'd like to have confirmation so 9f277c80258b3d2951128ce26a07c30ad0b47af0 (CLJ-979) can be excluded as the culprit

Comment by Alex Miller [ 11/Jan/15 8:09 AM ]

Correct.

Comment by Nicola Mometto [ 11/Jan/15 8:15 AM ]

Well this is weird then.
The only way I can think of that would produce that exception is if this returned nil: https://github.com/clojure/clojure/blob/3e7cb1a5c840612ad41cf6e0be92480f798bc05d/src/clj/clojure/core_deftype.clj#L57

This means a scenario like this:

user=> (defrecord x [] +)
NullPointerException   clojure.lang.Compiler.maybeResolveIn (Compiler.java:7015)
user=> (.printStackTrace *e)
java.lang.NullPointerException, compiling:(NO_SOURCE_FILE:3:1)
	at clojure.lang.Compiler.macroexpand1(Compiler.java:6620)
	at clojure.lang.Compiler.macroexpand(Compiler.java:6678)
	at clojure.lang.Compiler.eval(Compiler.java:6752)
	at clojure.lang.Compiler.eval(Compiler.java:6731)
	at clojure.core$eval.invoke(core.clj:3076)
	at clojure.main$repl$read_eval_print__7044$fn__7047.invoke(main.clj:239)
	at clojure.main$repl$read_eval_print__7044.invoke(main.clj:239)
	at clojure.main$repl$fn__7053.invoke(main.clj:257)
	at clojure.main$repl.doInvoke(main.clj:257)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.main$repl_opt.invoke(main.clj:323)
	at clojure.main$main.doInvoke(main.clj:421)
	at clojure.lang.RestFn.invoke(RestFn.java:397)
	at clojure.lang.Var.invoke(Var.java:375)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.NullPointerException
	at clojure.lang.Compiler.maybeResolveIn(Compiler.java:7015)
	at clojure.core$ns_resolve.invoke(core.clj:4200)
	at clojure.core$ns_resolve.invoke(core.clj:4197)
	at clojure.core$resolve.invoke(core.clj:4206)
	at clojure.core$map$fn__4523.invoke(core.clj:2612)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	at clojure.lang.RT.seq(RT.java:485)
	at clojure.core$seq__4103.invoke(core.clj:135)
	at clojure.core$reduce1.invoke(core.clj:899)
	at clojure.core$set.invoke(core.clj:3944)
	at clojure.core$emit_defrecord.invoke(core_deftype.clj:154)
	at clojure.core$defrecord.doInvoke(core_deftype.clj:374)
	at clojure.lang.RestFn.invoke(RestFn.java:497)
	at clojure.lang.Var.invoke(Var.java:401)
	at clojure.lang.AFn.applyToHelper(AFn.java:171)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.lang.Compiler.macroexpand1(Compiler.java:6607)
	... 16 more
nil

where a var is used as a protocol but no interface is present.

Comment by Nicola Mometto [ 11/Jan/15 8:18 AM ]

Alex, since I cannot reproduce, can you try getting the exception with a patched version of clojure that replaces https://github.com/clojure/clojure/blob/3e7cb1a5c840612ad41cf6e0be92480f798bc05d/src/clj/clojure/core_deftype.clj#L57
with something like

(or (:on (deref (resolve %))) 
    (println % @(resolve %)))

so we can get an idea of what's going on?

Comment by Nicola Mometto [ 11/Jan/15 11:38 AM ]

I was just able to reproduce this issue using clojure at commit 4afd4a7c14c48b5baf3c03196053066483cb4223

This means that CLJ-1544 is not responsable for this bug.

I can also confirm that this bug is intermittent, which makes figuring out what's going on really hard.

Comment by Nicola Mometto [ 11/Jan/15 12:10 PM ]

I still have absolutely no idea how this can happen but adding a bunch of printlns it turned out that for some reason in this binding of the deftype macro:

[interfaces methods opts] (parse-opts+specs opts+specs)

when opts+specs is nil, interfaces is sometimes [nil] as opposed to [].

This makes me think that there's some concurrency bug in the recent changes around the handling of vec, but this is just a guess.

Comment by Nicola Mometto [ 11/Jan/15 12:13 PM ]

I've restricted it down a bit and it looks like this part of opts+spec can bind interfaces to [nil] when impls is {}

interfaces (→ (map #(if (var? (resolve %))
                      (:on (deref (resolve %)))
                      %)
                   (keys impls))
             set
             (disj 'Object 'java.lang.Object)
             vec)
Comment by Nicola Mometto [ 11/Jan/15 12:25 PM ]

Here's an example output from my debugging tests, with the following patch applied:

diff --git a/src/clj/clojure/core_deftype.clj b/src/clj/clojure/core_deftype.clj
index 97e14cc..8f521eb 100644
--- a/src/clj/clojure/core_deftype.clj
+++ b/src/clj/clojure/core_deftype.clj
@@ -60,6 +60,8 @@ (defn- parse-opts+specs [opts+specs]
                        set
                        (disj 'Object 'java.lang.Object)
                        vec)
+        _ (when (nil? opts+specs)
+            (println impls interfaces))
         methods (map (fn [[name params & body]]
                        (cons name (maybe-destructured params body)))
                      (apply concat (vals impls)))]
{} [nil]
Exception in thread "main" java.lang.NullPointerException, compiling:(schema/utils.clj:68:1)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6716)
	at clojure.lang.Compiler.analyze(Compiler.java:6500)
	at clojure.lang.Compiler.analyze(Compiler.java:6461)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5837)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6155)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6709)
	at clojure.lang.Compiler.analyze(Compiler.java:6500)
	at clojure.lang.Compiler.analyze(Compiler.java:6461)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5837)
	at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5272)
	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3901)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6707)
	at clojure.lang.Compiler.analyze(Compiler.java:6500)
	at clojure.lang.Compiler.eval(Compiler.java:6765)
	at clojure.lang.Compiler.load(Compiler.java:7195)
	at clojure.lang.RT.loadResourceScript(RT.java:370)
	at clojure.lang.RT.loadResourceScript(RT.java:361)
	at clojure.lang.RT.load(RT.java:440)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5424.invoke(core.clj:5848)
	at clojure.core$load.doInvoke(core.clj:5847)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$load_lib$fn__5373.invoke(core.clj:5693)
	at clojure.core$load_lib.doInvoke(core.clj:5692)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5731)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5814)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at plumbing.core$eval13998$loading__5316__auto____13999.invoke(core.clj:1)
	at plumbing.core$eval13998.invoke(core.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6768)
	at clojure.lang.Compiler.eval(Compiler.java:6757)
	at clojure.lang.Compiler.load(Compiler.java:7195)
	at clojure.lang.RT.loadResourceScript(RT.java:370)
	at clojure.lang.RT.loadResourceScript(RT.java:361)
	at clojure.lang.RT.load(RT.java:440)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5424.invoke(core.clj:5848)
	at clojure.core$load.doInvoke(core.clj:5847)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$load_lib$fn__5373.invoke(core.clj:5693)
	at clojure.core$load_lib.doInvoke(core.clj:5692)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5731)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5814)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at user.fus_threading$eval13994.invoke(fus_threading.clj:6)
	at clojure.lang.Compiler.eval(Compiler.java:6768)
	at clojure.lang.Compiler.load(Compiler.java:7195)
	at clojure.lang.RT.loadResourceScript(RT.java:370)
	at clojure.lang.RT.loadResourceScript(RT.java:361)
	at clojure.lang.RT.load(RT.java:440)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5424.invoke(core.clj:5848)
	at clojure.core$load.doInvoke(core.clj:5847)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$load_lib$fn__5373.invoke(core.clj:5693)
	at clojure.core$load_lib.doInvoke(core.clj:5692)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5731)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5814)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at midje.repl$load_facts$fn__6148.invoke(repl.clj:206)
	at midje.repl$load_facts.doInvoke(repl.clj:192)
	at clojure.lang.RestFn.invoke(RestFn.java:397)
	at user$eval6211.invoke(form-init7109545842773565024.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6768)
	at clojure.lang.Compiler.eval(Compiler.java:6758)
	at clojure.lang.Compiler.load(Compiler.java:7195)
	at clojure.lang.Compiler.loadFile(Compiler.java:7151)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$init_opt.invoke(main.clj:279)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.NullPointerException
	at clojure.lang.Compiler.resolveIn(Compiler.java:6971)
	at clojure.lang.Compiler.resolve(Compiler.java:6949)
	at clojure.lang.Compiler$NewInstanceExpr.build(Compiler.java:7565)
	at clojure.lang.Compiler$NewInstanceExpr$DeftypeParser.parse(Compiler.java:7490)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6709)
	... 91 more
Comment by Nicola Mometto [ 11/Jan/15 12:32 PM ]

Further debugging is convincing me further that some of the recent changes around `vec` are causing this bug.

With the following patch:

diff --git a/src/clj/clojure/core_deftype.clj b/src/clj/clojure/core_deftype.clj
index 97e14cc..9478b04 100644
--- a/src/clj/clojure/core_deftype.clj
+++ b/src/clj/clojure/core_deftype.clj
@@ -53,13 +53,16 @@ (defn- parse-impls [specs]
 (defn- parse-opts+specs [opts+specs]
   (let [[opts specs] (parse-opts opts+specs)
         impls (parse-impls specs)
-        interfaces (-> (map #(if (var? (resolve %)) 
-                               (:on (deref (resolve %)))
-                               %)
-                            (keys impls))
-                       set
-                       (disj 'Object 'java.lang.Object)
-                       vec)
+        ks (keys impls)
+        interfaces' (map #(if (var? (resolve %))
+                            (:on (deref (resolve %)))
+                            %)
+                         ks)
+        interfaces'' (set interfaces')
+        interfaces''' (disj interfaces'' 'Object 'java.lang.Object)
+        interfaces (vec interfaces''')
+        _ (when (nil? opts+specs)
+            (println impls ks interfaces' interfaces'' interfaces''' interfaces))
         methods (map (fn [[name params & body]]
                        (cons name (maybe-destructured params body)))
                      (apply concat (vals impls)))]

I get this println when the NPE occurs:

{} nil () #{} #{} [nil]

Meaning that for some reson, `vec` of `#{}` returns `[nil]` rather than `[]`

Comment by Nicola Mometto [ 11/Jan/15 12:42 PM ]

I confirmed that the bug is in the vec function.
With the following patch, when the NPE occurs, the debug println is triggered:

diff --git a/src/jvm/clojure/lang/PersistentVector.java b/src/jvm/clojure/lang/PersistentVector.java
index 9804a0b..a460b6f 100644
--- a/src/jvm/clojure/lang/PersistentVector.java
+++ b/src/jvm/clojure/lang/PersistentVector.java
@@ -96,19 +96,22 @@ static public PersistentVector create(ArrayList list){
 static public PersistentVector create(Iterable items){
     // optimize common case
     if(items instanceof ArrayList)
         return create((ArrayList)items);

     Iterator iter = items.iterator();
     TransientVector ret = EMPTY.asTransient();
     while(iter.hasNext())
         ret = ret.conj(iter.next());
-    return ret.persistent();
+    PersistentVector r = ret.persistent();
+    if (RT.seq(r) != null && RT.seq(items) == null)
+        System.out.println("bug");
+    return r;
 }

 static public PersistentVector create(Object... items){
        TransientVector ret = EMPTY.asTransient();
        for(Object item : items)
                ret = ret.conj(item);
        return ret.persistent();
 }
Comment by Alex Miller [ 11/Jan/15 1:47 PM ]

And items is a PersistentSet? I've actually been looking at some weirdness on set iterators in the context of CLJ-1499 in consistency between seq and iterators.

Comment by Nicola Mometto [ 11/Jan/15 1:52 PM ]

I believe I have identified the bug, but I cannot make any sense out of it.

The bug apperas to be in SeqIterator.hasNext(), when the NPE occurs, after applying the following patch:

diff --git a/src/jvm/clojure/lang/SeqIterator.java b/src/jvm/clojure/lang/SeqIterator.java
index e6ad481..031fbc8 100644
--- a/src/jvm/clojure/lang/SeqIterator.java
+++ b/src/jvm/clojure/lang/SeqIterator.java
@@ -35,14 +35,18 @@ public SeqIterator(ISeq o){
 public boolean hasNext(){
        if(seq == this){
                seq = START;
                next = RT.seq(next);
                }
        else if(seq == next)
                next = RT.next(seq);
+    else if (RT.seq(next) == null)
+        System.out.println("this shouldn't happen: " + (this == seq));
+    if (RT.seq(next) == null && next != null)
+        System.out.println("bug: " + next);
        return next != null;
 }

 public Object next() throws NoSuchElementException {
        if(!hasNext())
                throw new NoSuchElementException();
        seq = next;

I get the following output:

this shouldn't happen: true
bug: #{}

I have absolutely no idea how it is possible that the last branch gets executed since it is true that seq == this thus the first branch should have been executed.

Comment by Nicola Mometto [ 11/Jan/15 1:58 PM ]

I don't know why, but with the attached patch the bug seems to go away.
This is probably just by accident though as I have no idea what changes between the code pre patch and the code post patch.

Comment by Alex Miller [ 11/Jan/15 3:21 PM ]

Without looking at the patch Id say that non deterministic bug plus impossible state smells like a concurrency / race condition problem.

Comment by Michael Blume [ 11/Jan/15 4:18 PM ]

This isn't the bug, per se, the thing I'm describing should not break anything, but why is the PersistentVector(Iterable) constructor being called on a PersistentHashSet? It looks like we could very easily do

--- i/src/jvm/clojure/lang/LazilyPersistentVector.java
+++ w/src/jvm/clojure/lang/LazilyPersistentVector.java
@@ -26,7 +26,7 @@ static public IPersistentVector createOwning(Object... items){
 static public IPersistentVector create(Object obj){
    if(obj instanceof IReduceInit)
        return PersistentVector.create((IReduceInit) obj);
-   else if(obj instanceof ISeq)
+   else if(obj instanceof Seqable)
        return PersistentVector.create(RT.seq(obj));
    else if(obj instanceof Iterable)
        return PersistentVector.create((Iterable)obj);

and treat the set directly as a seq. Is there some way that would be slower?

Comment by Michael Blume [ 11/Jan/15 4:45 PM ]

It may be that I've just tried it an insufficient number of times, but simply adding 'synchronized' to SeqIterator.hasNext appears to solve the problem. Again, this doesn't really tell us what the problem is.

ETA: Nope, fails sometimes even with synchronized.

Comment by Alex Miller [ 12/Jan/15 8:57 AM ]

The use of ISeq and not Seqable in LazilyPersistentVector is quite intentional. The idea here is that if something is already a seq (effectively a linked list), the best we're going to do is walk that chain. However, many things are Seqable that may have more efficient Iterable implementations which can (statefully) walk a data structure without creating all the intermediate objects required by seq. In particular, via CLJ-1499, map and set will soon be gaining direct Iterable implementations that walk the persistent tree without instantiating a seq object for every element. However, at the moment set and map will create a SeqIterator wrapped around the seq.

CLJ-1546 changed this path - it was walking the seq but is now walking it via SeqIterator. My working theory is that that switch has uncovered a latent race condition in SeqIterator that was never noticed before as the path wasn't exercised.

Note that because CLJ-1499 removes the reliance on SeqIterator, it would have avoided the bug in a different way! However, I have been seeing a number of weird things while doing dev on CLJ-1499 specifically around iterating over sets - the OO around iterator() and seq() in the APersistentSet/PersistentHashSet/PersistentTreeSet has some weird interactions.

I'm going to look a little closer at the suggested patch.

Comment by Nicola Mometto [ 12/Jan/15 9:01 AM ]

Alex, before you waste your time on my patch I want to clarify that I don't think that patch fixes the issue in any way, it's just a random change I made that happens to make the symptoms disappear on my system, I just attached it for debugging purposed.

Maybe there's a reason why the patch solves the bug or maybe it's just masking it, I still can't figure out why this apparent race condition happens.

Comment by Alex Miller [ 12/Jan/15 9:05 AM ]

Gotcha, thx.

Comment by Michael Blume [ 12/Jan/15 11:54 AM ]

Aha, makes sense, thanks =)

Comment by Andy Fingerhut [ 12/Jan/15 12:32 PM ]

I haven't dug into this, and don't have a solution, but SeqIterator's fields are not final, so there is no guarantee that the values assigned to a new instance's fields in its constructors will be visible to other threads, yes? And I believe that if those writes to the fields do eventually become visible, they need not become visible in the order that the assignments occur in the source code, but can become visible in any order.

Comment by Nicola Mometto [ 12/Jan/15 5:13 PM ]

By the way, I've been able to reproduce this bug using jdk 1.8 so it's not just with 1.7

Comment by Michael Blume [ 12/Jan/15 9:10 PM ]

SeqIterator seems to use 'this' as a sentinel value. If I replace it with an explicit 'new Object' sentinel, the problem appears to go away (~40 compilations without an NPE).

Making seq and next volatile doesn't help.

Interestingly, when I synchronize the entire SeqIterator class (both hasNext and next synchronized on this), the problem doesn't go away, so if this is a race condition, it's kind of a weird one.

I can insert a call to seq before the call to set here https://github.com/clojure/clojure/blob/clojure-1.7.0-alpha5/src/clj/clojure/core_deftype.clj#L60 and the problem doesn't go away. I can then print the result of seq before it's passed to set, and, of course, it's a nil.

So somehow, we're basically evaluating (-> nil set (disj 'Object 'java.lang.Object) vec) and getting [nil] instead of []

But when I actually evaluate that expression in a REPL (from 20 threads at once, 1M times in a row) it evaluates to empty vector every time.

So I'm confused.

Debug patch here if anyone wants to check my work: https://gist.githubusercontent.com/MichaelBlume/735c8f601210cfa1ecaf/raw/814f21e5e4abb2ca9d1d5330d0b4cc2b3a4424e6/gistfile1.txt

Comment by Alex Miller [ 12/Jan/15 9:53 PM ]

I'm with you. I'm starting to suspect that this is involved:

https://github.com/clojure/clojure/commit/43cc1854508d655e58e377f84836ba128971f90c

Comment by Andy Fingerhut [ 12/Jan/15 11:57 PM ]

Out of curiosity, I tried adding a field to the SeqIterator class that remembers the thread that constructed each instance, and then checks in the call to hasNext if the calling thread is the same as the creator thread, printing a message if they are ever different. I never saw that while building Clojure, nor running the command on the Midje project. That seems to rule out the possibility of the SeqIterator getting passed from one thread to another.

If that is always true, then I'm with Nicola: I would love an explanation of what is going on here to cause the debug print's he mentions in a comment above to print what they do when a failure occurs. It looks really, really wrong, as in wrong single-threaded program behavior.

Comment by Nicola Mometto [ 13/Jan/15 5:09 AM ]

Michael's and Andy's findings agree with what I observed. There's no multithreading involved yet somehow there's what appears to be a data race condition in SeqIterator.

Also, as Michael observed in his own testings, changing the sentinel from this to an Object instance (START) makes the issue go away, this is exactly what the patch I attached does.

I am also unable to reproduce this issue by repeatedly invoking vec on an empty set.

Comment by Nicola Mometto [ 13/Jan/15 5:15 AM ]

This just happened:

public boolean hasNext(){
    Object a = seq;
    Object b = this;
	if(seq == this){
		seq = START;
		next = RT.seq(next);
		}
	else {
        if (RT.seq(next) == null) {
            System.out.println (a);
            System.out.println (b);
        }
        if(seq == next)
            next = RT.next(seq);
    }
	return next != null;
}
clojure.lang.SeqIterator@3ddc6873
clojure.lang.SeqIterator@3ddc6873
Exception in thread "main" java.util.NoSuchElementException, compiling:(utils.clj:68:1)
Comment by Nicola Mometto [ 13/Jan/15 5:45 AM ]

I'm taking a shot in the dark but I tried reproducing this bug using -Xint (using the jvm interpreter) and I can't seem to be able to reproduce it after many runs.
As absurd as this sounds, I'm starting to think that some hotspot optimization is responsible for this nonsensical behaviour – this would explain the nondeterministic behaviour in the absence of multithreading.

Comment by Alex Miller [ 13/Jan/15 8:50 AM ]

I did some experiments myself yesterday and found many of the same things listed here - single-threaded usage with seemingly impossible results.

I am currently thinking that setting seq = this; in the constructor is unsafe publication of this. "this" is not valid until after the constructor completes, yet we have saved away a reference to it in a field.

Thus seq==this may possibly return false in hasNext() because it is comparing a partially constructed object with the fully constructed object (same object identity!). It may be that this doesn't happen until after hot spot/inlining. By turning on the inline diagnostics, I do see these SeqIterator methods as a hot spot that gets inlined at some point soon before I see the error manifest.

This is my best explanation of the results we're seeing and seems sufficient justification to change the implementation of SeqIterator to me.

Comment by Nicola Mometto [ 13/Jan/15 8:59 AM ]

From the Java Language Spec, section 15.8.3:
"The keyword this may be used only in the body of an instance method or default method, or in the body of a constructor of a class, or in an instance initializer of a class, or in the initializer of an instance variable of a class"

So I don't believe that the current usage of this is incorrect for this reason, however I agree that it's likely caused by some interaction of doing identity check with this and hot spot/inlining issue so I'm more confident that the patch I posted is a right fix for this bug.

Comment by Andy Fingerhut [ 13/Jan/15 10:30 AM ]

The only other scenario of multiple calls to next / hasNext that I could think of with a single thread is if some method call inside of hasNext causes next / hasNext to be called, nested, on the same SeqIterator instance. I did some instrumentation to check for nested calls, unlikely as that would seem from the source code, and saw a failure with no nested call to hasNext.

Comment by Alex Miller [ 13/Jan/15 4:28 PM ]

Midje stack trace (removed from original ticket, left here for safe-keeping). To reproduce:

// set your JAVA_HOME and PATH to JDK 1.7
git clone git@github.com:marick/Midje.git
cd Midje
git co e98cf87
lein with-profile 1.7 midje

Error:

Exception in thread "main" java.lang.NullPointerException, compiling:(fim_collection_diffs.clj:7:1)
	at clojure.lang.Compiler.macroexpand1(Compiler.java:6619)
	at clojure.lang.Compiler.macroexpand(Compiler.java:6677)
	at clojure.lang.Compiler.eval(Compiler.java:6751)
	at clojure.lang.Compiler.load(Compiler.java:7194)
	at clojure.lang.RT.loadResourceScript(RT.java:370)
	at clojure.lang.RT.loadResourceScript(RT.java:361)
	at clojure.lang.RT.load(RT.java:440)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5436.invoke(core.clj:5863)
	at clojure.core$load.doInvoke(core.clj:5862)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$load_lib$fn__5383.invoke(core.clj:5708)
	at clojure.core$load_lib.doInvoke(core.clj:5707)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5746)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5829)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at midje.repl$load_facts$fn__6148.invoke(repl.clj:206)
	at midje.repl$load_facts.doInvoke(repl.clj:192)
	at clojure.lang.RestFn.invoke(RestFn.java:397)
	at user$eval6211.invoke(form-init3965655274254111851.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6767)
	at clojure.lang.Compiler.eval(Compiler.java:6757)
	at clojure.lang.Compiler.load(Compiler.java:7194)
	at clojure.lang.Compiler.loadFile(Compiler.java:7150)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$init_opt.invoke(main.clj:279)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.NullPointerException
	at clojure.lang.Compiler.maybeResolveIn(Compiler.java:7014)
	at clojure.core$ns_resolve.invoke(core.clj:4200)
	at clojure.core$ns_resolve.invoke(core.clj:4197)
	at clojure.core$resolve.invoke(core.clj:4206)
	at clojure.core$map$fn__4529.invoke(core.clj:2612)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	at clojure.lang.RT.seq(RT.java:485)
	at clojure.core$seq__4109.invoke(core.clj:135)
	at clojure.core$reduce1.invoke(core.clj:899)
	at clojure.core$set.invoke(core.clj:3944)
	at clojure.core$emit_defrecord.invoke(core_deftype.clj:154)
	at clojure.core$defrecord.doInvoke(core_deftype.clj:374)
	at clojure.lang.RestFn.invoke(RestFn.java:470)
	at clojure.lang.Var.invoke(Var.java:394)
	at clojure.lang.AFn.applyToHelper(AFn.java:165)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.lang.Compiler.macroexpand1(Compiler.java:6606)
Comment by Ghadi Shayban [ 13/Jan/15 4:32 PM ]

Not sure I fully understand the sad path that causes this bug, but START can safely be marked static final in the patch.

Comment by Andy Fingerhut [ 13/Jan/15 4:54 PM ]

Does this seem like it may be a bug in JIT compilation to anyone? I ask because as far as we can tell, this bug occurs completely within a single thread, and as far as I've read, the Java memory model should guarantee that operations in a single thread appear to occur in the order they happen in the source code.

Independent question: It seems that the assumption is that a SeqIterator is only ever accessed from 1 thread. Is it considered an internal implementation detail, and thus on documentation is needed about this assumption?

Comment by Alex Miller [ 13/Jan/15 5:22 PM ]

Re JIT - I wouldn't rule that out, but if it is, that doesn't help us make Clojure work again for everyone with existing JVMs. JCiP section 3.2.1 says "If the this reference escapes during construction, the object is considered not properly constructed." which sounds like what we're doing.

Re threading - I think that the use of iterators inside Clojure itself has (until recently) been unusual. Virtually everything is written to leverage the seq model and iterators were largely provided for Java compliance. With the creation of LazyTransformer and extension of reduce to iterators, this orientation has changed somewhat. However, I'd say that iterators are dirty stateful things and they should be consumed in localized contexts by no more than one thread at a time. If they are used in a thread-confined way and safely published between threads, SeqIterator seems ok.

Comment by Nicola Mometto [ 13/Jan/15 5:32 PM ]

0001-CLJ-1636-don-t-use-this-as-a-sentinel-in-SeqIterator.patch is the same as 0001-fix-for-CLJ-1636.patch except it makes START final as suggested by Ghadi

Comment by Ghadi Shayban [ 15/Jan/15 12:27 PM ]

Alex has screened this – which probably implies that the patch fixes the issue. Just to confirm does the patch clear up the issue for everyone else?

Comment by Michael Blume [ 15/Jan/15 12:48 PM ]

Does for me, yes =)

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

If you want to collect test results, seems like it would be good for people to respond with the OS version and JVM version they tested on, and whether it was the Midje test in the description of this ticket that they tried, or some other test.

Comment by Andy Fingerhut [ 16/Jan/15 4:59 PM ]

Out of curiosity, does anyone have a smaller test case that causes this incorrect return value from the SeqIterator, without running the 'lein with-profile 1.7 midje' command on Midje? e.g. running a page or less worth of Clojure code?

Comment by Alex Miller [ 16/Jan/15 6:01 PM ]

While we've applied the patch, I would still love to understand wtf is happening here and would love to see that too. For me, I can quite reliably reproduce it building Clojure itself. To support the theory of it happening during an inlining transition, it's unlikely to reproduce outside the context of other code however. I see it get embedded inside a big wad of calling code when I watching inlining.

Comment by Nicola Mometto [ 18/Jan/15 11:36 AM ]

I've spent some time reading through both the jvm and the java specs and I can't find a reasonable explaination for what was happening, I can only think this is a bug in some hotspot inlining optimization.





[CLJ-1635] Make distinct/dedupe/interpose transducer tests play nicely with new reporting Created: 09/Jan/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: test, transducers

Attachments: Text File clj-1635.patch    
Patch: Code and Test
Approval: Ok

 Description   

Fix interaction between CLJ-1601 (which introduced new transducers) and CLJ-1621 (which improved transducer tests) to improve test reporting for these new transducer arities as well.

Note from Alex M: I goofed these while rebasing CLJ-1601 after CLJ-1621 went in.

Patch: clj-1635.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 09/Jan/15 2:31 PM ]

My fault in the integration process! We'll try to get it fixed in 1.7. Thanks...





[CLJ-1634] Potential bug in trampoline Created: 07/Jan/15  Updated: 07/Jan/15  Resolved: 07/Jan/15

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

Type: Defect Priority: Major
Reporter: Hongseok Yang Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: bug, trampoline
Environment:

Java HotSpot(TM) 64-Bit Server VM 1.8.0_25-b17



 Description   

Hi,

While trying to use trampoline to optimise tail recursion in my Clojure project, I came across some strange behaviour of the trampoline function.

=> (defn f [g] (fn [k & args] #(k (apply g args))))
...
=> (trampoline (f list) println 1 2 3)
(#<core$println clojure.core$println@54e517f6> 1 2 3)
nil
=> (((f list) println 1 2 3))
(1 2 3)
nil

I think that (trampoline (f list) ...) and ((f list) ...) should give the same result.

In fact, I asked this question in Stackoverflow before. You can find some further discussion about this potential bug.

https://stackoverflow.com/questions/27819418/strange-behaviour-of-clojure-trampoline

Best wishes,
Hongseok



 Comments   
Comment by Nicola Mometto [ 07/Jan/15 11:48 AM ]

I opened CLJ-1633 just a few moments before you with a patch fixing this issue.





[CLJ-1633] PersistentList/creator doesn't handle ArraySeqs correctly Created: 07/Jan/15  Updated: 10/Jan/15  Resolved: 10/Jan/15

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: collections

Attachments: Text File 0001-CLJ-1633-fix-PersistentList-creator-handling-of-Arra.patch     Text File CLJ-1633-v2.patch     Text File CLJ-1633-v3.patch     Text File generative-seq-tests.patch     Text File generative-seq-tests-v2.patch    
Patch: Code and Test
Approval: Ok

 Description   

This should return '(2 3) but returns '(1 2 3) instead:

user=> ((fn [& args] (apply (fn [a & b] (println a b) (apply list b)) args)) 1 2 3)
1 (2 3)
(1 2 3)

Note that using vector rather than list returns the correct values:

user=> ((fn [& args] (apply (fn [a & b] (println a b) (apply vector b)) args)) 1 2 3)
1 (2 3)
[2 3]

The bug was reported in this stackoverflow question: https://stackoverflow.com/questions/27819418/strange-behaviour-of-clojure-trampoline and the bug identified in this comment: https://stackoverflow.com/questions/27819418/strange-behaviour-of-clojure-trampoline#comments-27821793

A simpler example of this bug:

user=> (apply list (next (clojure.lang.ArraySeq/create (object-array [1 2 3]))))
(1 2 3)

Patch: CLJ-1633-v3.patch



 Comments   
Comment by Michael Blume [ 07/Jan/15 12:28 PM ]

Very nice catch.

This makes me wonder if there's more we can do with generative testing to catch this class of bugs, maybe along the lines of zach tellman's collection-check

Comment by Alex Miller [ 07/Jan/15 2:27 PM ]

There's definitely more we can do. collection-check is great and I've started to integrate some of those ideas into Clojure's tests (see for example the new transducer tests that generate random chains of sequence operations and compare seq and transducer executions). If you have ideas about specific test areas, would be happy to see a jira/patch.

Comment by Michael Blume [ 07/Jan/15 4:59 PM ]

Updated test to get expected list inside the (= ...) form

Comment by Michael Blume [ 07/Jan/15 5:03 PM ]

Oops, rerolling again to apply cleanly to master

Comment by Nicola Mometto [ 07/Jan/15 5:04 PM ]

Thanks for the fix!

Comment by Michael Blume [ 07/Jan/15 5:40 PM ]

No problem =)

Comment by Michael Blume [ 07/Jan/15 5:42 PM ]

Ok, this is kind of crude, and mostly a proof of concept, but this does catch the bug.

Output:

[java] FAIL in (seq-gentest) (sequences.clj:105)
     [java] {:acts1 (-> [] next (->> (cons :foo)) (->> (cons :foo)) next),
     [java]  :acts2
     [java]  (->
     [java]   []
     [java]   next
     [java]   (->> (cons :foo))
     [java]   (->> (cons :foo))
     [java]   into-array-seq
     [java]   next
     [java]   (->> (apply list))),
     [java]  :result1 (:foo),
     [java]  :result2 (:foo :foo),
     [java]  :pass false}
     [java]
     [java] expected: (:result res)
     [java]   actual: false
Comment by Alex Miller [ 09/Jan/15 9:12 AM ]

Rich said to move this forward.

Comment by Nicola Mometto [ 09/Jan/15 5:51 PM ]

This ticket has been closed but no patch has been committed

Comment by Alex Miller [ 09/Jan/15 6:22 PM ]

Stu, doesn't look like this patch was applied but it was closed?





[CLJ-1632] Mark Clojure-generated classes in order for instrumenters to identify them easily Created: 05/Jan/15  Updated: 05/Jan/15  Resolved: 05/Jan/15

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

Type: Enhancement Priority: Minor
Reporter: Fabio Tudone Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: compiler


 Description   

Instrumenting logic specific to Clojure-generated classes should be able to identify them easily.



 Comments   
Comment by Fabio Tudone [ 05/Jan/15 2:23 AM ]

One way could be annotations, another could be interface-marking. Would that be feasible? Any drawbacks?

Comment by Alex Miller [ 05/Jan/15 8:17 AM ]

deftypes have a marker interface clojure.lang.IType.
defrecords have a marker interface clojure.lang.IRecord.
proxy classes have marker interface clojure.lang.IProxy.

I think generic markers for protocols or gen-interface would be undesirable as they may be used to create APIs for external use.

Comment by Fabio Tudone [ 05/Jan/15 8:29 AM ]

Not sure I understand your point about the non-marked (as of now and AFAIK) Clojure features such as protocols and gen-interface; could you elaborate? Does it apply to marking with annotations as well?

Comment by Alex Miller [ 05/Jan/15 10:50 AM ]

My point was that many people wish to generate interfaces that do not extend from interfaces in Clojure and adding those marker interfaces would be seen as a downside for them. Annotations are slightly better but have the same problem (dependencies on parts of Clojure core). You are of course free to add those interfaces or annotations yourself in your own code if that's useful to you!

Comment by Fabio Tudone [ 05/Jan/15 11:15 AM ]

It's clear now, thanks!

Actually my use case is about general tooling that will inspect and instrument all (and only) Clojure-generated code in any application making use of it, so I don't control the code I'm going to examine. This is done in order to add specific runtime features in a general fashion.

I can't find a way to do that reliably on everything that has been generated by Clojure; I could use some imperfect heuristics but I'd rather use a reliable way if one exists. Can you see of any other way of doing this I might have overlooked? Or is some other enhancement possible that would allow me to do this and would not compromise external integrations?

Comment by Alex Miller [ 05/Jan/15 11:31 AM ]

I don't see a general-purpose way to do this now. I do not believe supporting this is something we would spend time on.

Comment by Julien Eluard [ 05/Jan/15 2:03 PM ]

Class#isSynthetic() might be relevant here. If I am not mistaken asm generated classes will be flagged as synthetic.





[CLJ-1631] Issue with type hints on return values Created: 01/Jan/15  Updated: 01/Jan/15  Resolved: 01/Jan/15

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

Type: Defect Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

When a return value is type hinted in one namespace, errors can occur when the function is used from another namespace. Simple code to reproduce:

(ns foo (:import [java.awt.image BufferedImage]))

(defn f ^BufferedImage []
(BufferedImage. (int 10) (int 10) BufferedImage/TYPE_INT_ARGB))

(ns bar (:require foo))

(.getType (foo/f))
=> CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: BufferedImage, compiling:(NO_SOURCE_PATH:1:1)

I would expect any usage of foo/f to correctly use the fully qualified class name (java.awt.image.BufferedImage). Note that if I add an extra type hint at the call site, it seems to work OK:

(.getType ^java.awt.image.BufferedImage (foo/f))
=> 2



 Comments   
Comment by Nicola Mometto [ 01/Jan/15 4:32 AM ]

Duplicate of http://dev.clojure.org/jira/browse/CLJ-1232





[CLJ-1630] Destructuring allows multiple &-params Created: 31/Dec/14  Updated: 01/Jan/15

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

Type: Defect Priority: Major
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File no-multiple-rest-params-v1.patch    
Patch: Code and Test

 Description   

(let [[foo & bar & baz] []]) compiles and probably shouldn't.



 Comments   
Comment by Alex Miller [ 01/Jan/15 10:17 AM ]

I see:

user=> (defn foo [bar & baz & qux])

CompilerException java.lang.RuntimeException: Invalid parameter list, compiling:(/private/var/folders/7r/_1fj0f517rgcxwx79mn79mfc0000gn/T/form-init3743582784321941885.clj:1:1)

?

Comment by Michael Blume [ 01/Jan/15 12:27 PM ]

Sorry, I was working on memory rather than actually typing the thing I put in the description into a REPL, which was dumb.

user=> (let [[foo & bar & baz] []])
nil





[CLJ-1629] Improve error message when defn form omits parameter declaration Created: 29/Dec/14  Updated: 29/Dec/14

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

Type: Enhancement Priority: Major
Reporter: Sanel Zukan Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs
Environment:

Reproducible on all platforms and all clojure versions.


Approval: Triaged

 Description   

When defn form is malformed, Clojure compiler will report meaningless error and in combination with function body, can cause really bad experience. Here is the sample:

(defn foo
  "This is docstring."
  (let [i 1]
    (+ i 1)))

It will report:

IllegalArgumentException Parameter declaration "let" should be a vector  clojure.core/assert-valid-fdecl (core.clj:7123)

However, if is written:

(defn foo "bla")

error report makes more sense:

IllegalArgumentException Parameter declaration missing  clojure.core/assert-valid-fdecl (core.clj:7107)


 Comments   
Comment by Michael Blume [ 29/Dec/14 1:39 PM ]

I don't think this is really meaningless – if you replace the symbol let with a vector, say, [i], you get a perfectly valid function definition

(defn foo
  "This is docstring."
  ([i] [i 1]
    (+ i 1)))
Comment by Sanel Zukan [ 29/Dec/14 2:41 PM ]

Yes and maybe make sense for this case. But in general, the report is misleading for common defn forms (how often you will see function definitions written this way, unless you want multi-arity function) and should have the same report as for second sample; in both cases it is the same cause.





[CLJ-1628] Accept list as lib specification in clojure.core/require and clojure.core/use Created: 28/Dec/14  Updated: 30/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Petr Gladkikh Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File NS-macro-accept-lists-as-libspecs.patch    
Patch: Code

 Description   

Currently function clojure.core/load-libs treats '(a.namespace.name) as prefix list so this construct has no effect at all (as if it was prefix list without suffixes). At the same time '[a.namespace.name] causes require call (require 'a.namespace.name). In other cases functions require or use are ambivalent about differences between [] and (). In this particular case there is difference between no-op and lib loading. E.g. Clojure validation tool Eastwood includes rule for this case since the behavior of (require '(a.namespace.name)) is not obvious.

The suggested change lets to avoid this special case in require or use calls (including ones that stem from ns macro expansion). Accepting both list and vector as library specification makes behavior uniform and similar to the way suffix items are handled in prefix lists.

The patch is minimal in order to avoid reordering sequential? functions in clojure/core.clj
Should I include tests for these cases also?



 Comments   
Comment by Petr Gladkikh [ 30/Dec/14 2:39 AM ]

If on the other hand representing prefix lists as Clojure lists is intentional and list-for-prefix, vector-for-libspec-or-suffix should be distinguishing feature then we should issue error when

  • prefix list is enclosed in Clojure vector
  • libspec or suffix is in Clojure list

If backwards compatibility is important then one may at least write a warning in ':verbose' mode.

Also there should be error or warning if prefix list is empty.





[CLJ-1627] Incorrect error message: "First argument to def must be a Symbol" Created: 27/Dec/14  Updated: 29/Dec/14  Resolved: 28/Dec/14

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

Type: Defect Priority: Trivial
Reporter: Richard Davies Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

All



 Description   

(def (symbol "x")) throws the exception: First argument to def must be a Symbol

Given the first argument to this def is a symbol, the error message is incorrect.

This has resulted in:

See http://stackoverflow.com/questions/2486752/in-clojure-how-to-define-a-variable-named-by-a-string

Which so far has >3000 views suggesting that this is something that regularly stumps people the first time they encounter it.

I suggest changing the error message to:

"First argument to def must be an interned Symbol"



 Comments   
Comment by Nicola Mometto [ 28/Dec/14 4:00 AM ]

The first argument to def is not a symbol, it's the list (symbol "x").
def is neither a macro nor a regular function, it's a special form whose first argument is not evaluated. This is documented.

Comment by Alex Miller [ 28/Dec/14 10:39 AM ]

The current error message is literally correct. I don't see how the requested change would help anyone confused by the prior message.

Comment by Richard Davies [ 29/Dec/14 8:46 PM ]

@Ah. It's not explicit in the docs (at least in http://clojure.org/special_forms) or the error message that the first arg isn't evaluated. The docs talk about the "init?" paramater and symbol metadata being evaluated but not that the first argument itself is not actually evaluated. This is probably obvious to those familiar with the workings of the Clojure compiler but as hacking around with vars isn't something I do every day it wasn't to me.

Compare def's error message with trying to do the same thing with var:

(var (symbol "a"))

clojure.lang.Compiler$CompilerException: java.lang.ClassCastException: clojure.lang.PersistentList cannot be cast to clojure.lang.Symbol

At least that gives me a hint that the expression wasn't evaluated rather than just a wtf moment...





[CLJ-1626] ns macro: compare ns name during macroexpansion. Created: 23/Dec/14  Updated: 23/Dec/14

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

Type: Enhancement Priority: Trivial
Reporter: Petr Gladkikh Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: File compare-ns-name-at-macroexpansion.diff    

 Description   

Macroexpansion of 'ns' produces 'if' form that is executed at runtime. However comparison can be done during macroexpansion phase producing clearer resulting form in most cases.

Patch for suggested change is in attachment.






[CLJ-1625] Cannot implement protocol methods of the same name inline Created: 23/Dec/14  Updated: 23/Dec/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: Tassilo Horn Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: protocols


 Description   

One major benefit of protocols (IMHO) is that the protocol methods are properly namespace qualified. Thus I can have multiple protocols in different namespaces that define a foo method and extend them all (or a subset of them) upon existing types. However, that's not true with extending protocols inline with defrecord and deftype, or with extending protocols on the Java side by implementing their interfaces.

Example:

;; file: protocoltest/foo.clj
(ns prototest.foo)
(defprotocol Foo
  (mymethod [this]))

;; file: protocoltest/bar.clj
(ns prototest.bar)
(defprotocol Bar
  (mymethod [this]))

;; file: protocoltest/core.clj
(ns prototest.core
  (:require [prototest.foo :as foo]
            [prototest.bar :as bar]))

;; inline extension of both mymethod methods doesn't work
(defrecord MyRec [x]
  foo/Foo
  (mymethod [this] :foo)
  bar/Bar
  (mymethod [this] :bar))
;;=> java.lang.ClassFormatError
;;   Duplicate method name&signature in class file prototest/core/MyRec

;; I have to resort to either half-inline-half-dynamic...
(defrecord MyRec [x]
  foo/Foo
  (mymethod [this] :foo))
(extend-type MyRec
  bar/Bar
  (mymethod [this] :bar))

;; ... or fully dynamic extension.
(defrecord MyRec [x])
(extend-type MyRec
  foo/Foo
  (mymethod [this] :foo)
  bar/Bar
  (mymethod [this] :bar))

;; Then things work just fine.
(foo/mymethod (->MyRec 1))
;;=> :foo
(bar/mymethod (->MyRec 1))
;;=> :bar

I know that I get the error because both the Foo and the Bar interfaces backing the protocols have a mymethod method and thus they cannot be implemented both at once (at least not with different behavior).

But why throw away the namespacing advantages we have with protocols? E.g., why is the protocoltest.foo.Foo method not named protocoltest$foo$mymethod (or some other munged name) in the corresponding interface? That way, both methods can be implemented inline where you gain the speed advantage, and you can do the same even from the Java side. (Currently, invoking clojure.core.extend from the Java side using clojure.java.api is no fun because you have to construct maps, intern keywords, define functions, etc.)

Of course, the ship of changing the default method naming scheme has sailed long ago, but maybe a :ns-qualified-method-names option could be added to defprotocol.






[CLJ-1624] Support get from arbitrary java.util.List data types Created: 23/Dec/14  Updated: 23/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop

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

 Description   

Currently "get", "get-in" and related functions in clojure.core work on Clojure vectors, maps and Java arrays, but do not work on instances of java.util.List

(def al (java.util.Arrays/asList (object-array [1 2 3 4])))
(get al 2)
=> nil

This makes it inconvenient to work with nested structures of Java objects that could otherwise be viewed as similar to nested Clojure data structures.

This is also inconsistent with other clojure.core functions that do support arbitrary java.util.List instances (e.g. "nth" and "count")

With a small change to RT.java, it is possible to allow core functions to operate on arbitrary instances of java.util.List. There does not appear to be any significant downside to this change (it is not on the fast path so will not affect regular ILookup or Map checks).



 Comments   
Comment by Mike Anderson [ 23/Dec/14 12:31 AM ]

Patch for CLJ-1624





[CLJ-1623] Support zero-depth structures for update and update-in Created: 22/Dec/14  Updated: 24/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Currently "update" and "update-in" assume a nested associative structure at least 1 level deep.

For greater generality, it would be preferable to also support the case of 0 levels deep (i.e. a nested associative structure where there is only a leaf node)

examples:

;; Zero-length paths would be supported in update-in
(update-in 1 [] inc) => 2

;; update would get an extra arity which simply substitutes the new value
(update :old :new) => :new



 Comments   
Comment by Ghadi Shayban [ 23/Dec/14 7:56 AM ]

Duplicate of CLJ-373 which has been declined?

Comment by Alex Miller [ 23/Dec/14 8:19 AM ]

Rich has declined similar requests in the past.

Comment by Mike Anderson [ 23/Dec/14 7:50 PM ]

I disagree with the reasons for rejecting the previous patch. Can we revisit this?

Yes, it is a (very minor) behaviour change for update-in, but only only on undefined implementation behaviour, and even then only on the error case. If people are relying on this then their code is already very broken.

On the plus side, is makes the behaviour more logical and consistent. There is clearly demand for the change (see the various comments in favour of improving this in CLJ-373)

As an aside: if you really want to keep the old behaviour of disallowing empty paths then it would be better to convert the NullPointerException into a meaningful error message e.g. "Empty key paths are not allowed"

Also, I am proposing a corresponding change to update which doesn't have the above concern (since it is introducing a whole new arity)

Comment by Alex Miller [ 24/Dec/14 7:55 AM ]

Sorry, Rich has said he's not interested.





[CLJ-1622] No way to AOT class with interface or parent whose fully qualified name doesn't contain a period Created: 22/Dec/14  Updated: 22/Dec/14

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

Type: Defect Priority: Minor
Reporter: Mark Sikora Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: bug, interop


 Description   

I'm trying to interop with some java code. I have a class files in a jar that contains an interface called `Player` which I can't modify. The issue is that a bare name like this automatically has java.lang prepended to it by the gen-class macro. My ns looks something like

(ns PlayerAI
(:gen-class :implements [Player]))

which throws an error

java.lang.ClassNotFoundException: java.lang.Player, compiling:(PlayerAI.clj:1:1)

There is no way to avoid this based on what I see in the source here.






[CLJ-1621] Improve reporting in transducers generative test. Created: 21/Dec/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Enhancement Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: transducers

Attachments: Text File transducer-reporting-v1.patch    
Patch: Code and Test
Approval: Ok

 Description   

If the transducers generative test breaks, you get output like this:

[java] {:test-var seq-and-transducer, :result #<ExceptionInfo clojure.lang.ExceptionInfo: Applied actions to coll as seq, sequence transducer, and into transducer and got different results. {:coll [-16 10 -8 8 -5], :actions map clojure.core$dec@782a4056,take 5,partition-by clojure.core$even_QMARK_@2200d281,partition-all 9,map clojure.core$inc@643b798d,drop 9,remove clojure.core$empty_QMARK_@4600f352,remove clojure.core$odd_QMARK_@32dd05af, :s #<ClassCastException java.lang.ClassCastException: clojure.lang.LazySeq cannot be cast to java.lang.Number>, :xs (), :xi [], :xt []}>, :seed 1419199634890, :failing-size 21, :num-tests 22, :fail [[-16 10 -8 8 -5] [{:desc map clojure.core$dec@782a4056, :xf #<core$map$fn__3669 clojure.core$map$fn__3669@28449652>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@506b8505>} {:desc take 5, :xf #<core$take$fn__3712 clojure.core$take$fn__3712@38934406>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@27ce0b6d>} {:desc partition-by clojure.core$even_QMARK_@2200d281, :xf #<core$partition_by$fn__5568 clojure.core$partition_by$fn__5568@5287c159>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@70961c7b>} {:desc partition-all 9, :xf #<core$partition_all$fn__5590 clojure.core$partition_all$fn__5590@3f869b0>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@6f99ed9f>} {:desc map clojure.core$inc@643b798d, :xf #<core$map$fn__3669 clojure.core$map$fn__3669@2f2c41d3>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@2fdbef8d>} {:desc drop 9, :xf #<core$drop$fn__3728 clojure.core$drop$fn__3728@4f7b4b50>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@214b9b5>} {:desc remove clojure.core$empty_QMARK_@4600f352, :xf #<core$filter$fn__3696 clojure.core$filter$fn__3696@6846d654>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@7df231c7>} {:desc remove clojure.core$odd_QMARK_@32dd05af, :xf #<core$filter$fn__3696 clojure.core$filter$fn__3696@5a8ce6dd>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@34ee9000>}]], :shrunk {:total-nodes-visited 32, :depth 12, :result #<ExceptionInfo clojure.lang.ExceptionInfo: Applied actions to coll as seq, sequence transducer, and into transducer and got different results. {:coll [0], :actions map clojure.core$inc@643b798d, :s (1), :xs (0), :xi [0], :xt [0]}>, :smallest [[0] [{:desc map clojure.core$inc@643b798d, :xf #<core$map$fn__3669 clojure.core$map$fn__3669@2f2c41d3>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@2fdbef8d>}]]}}
     [java]
     [java] ERROR in (seq-and-transducer) (core.clj:4566)
     [java] Uncaught exception, not in assertion.
     [java] expected: nil
     [java]   actual: clojure.lang.ExceptionInfo: Applied actions to coll as seq, sequence transducer, and into transducer and got different results.
     [java]  at clojure.core$ex_info.invoke (core.clj:4566)
     [java]     clojure.test_clojure.transducers$seq_and_transducer_same_result.invoke (transducers.clj:103)
     [java]     clojure.lang.AFn.applyToHelper (AFn.java:156)
...etc etc

This has a few problems:

  • when clojure functions are given as arguments, they're full object printouts, with classnames and memory addresses, this is kind of hard to read
  • the combination of the first problem found with the shrunk version means there's a lot of content to read
  • lack of pretty printing makes that content very hard to read
  • the traceback isn't actually that helpful – we know what failed already.

Approach: The attached patch encodes more descriptive info in the actions and does a better job of reporting the difference in an understandable manner:

[java] FAIL in (seq-and-transducer) (transducers.clj:135)
     [java] {:coll [0],
     [java]  :actions (->> coll (map inc)),
     [java]  :s (1),
     [java]  :xs (0),
     [java]  :xi [0],
     [java]  :xt [0]}

Patch: transducer-reporting-v1.patch

Screened by: Alex Miller






[CLJ-1620] Constants are leaked in case of a reentrant eval Created: 18/Dec/14  Updated: 07/Jan/15

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

Type: Defect Priority: Critical
Reporter: Christophe Grand Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: aot, compiler

Attachments: Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer.patch     Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v2.patch     Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v3.patch     Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v4.patch     Text File eval-bindings.patch    
Patch: Code
Approval: Triaged

 Description   

Compiling a function that references a non loaded (or unitialized) class triggers its init static. When the init static loads clojure code, some constants (source code I think) are leaked into the constants pool of the function under compilation.

It prevented CCW from working in some environments (Rational) because the static init of the resulting function was over 64K.

Steps to reproduce:

Load the leak.main ns and run the code in comments: the first function has 15 extra fiels despite being identical to the second one.

(ns leak.main)

(defn first-to-load []
  leak.Klass/foo)

(defn second-to-load []
  leak.Klass/foo)

(comment
=> (map (comp count #(.getFields %) class) [first-to-load second-to-load])
(16 1)
)
package leak;
 
import clojure.lang.IFn;
import clojure.lang.RT;
import clojure.lang.Symbol;
 
public class Klass {
  static {
    RT.var("clojure.core", "require").invoke(Symbol.intern("leak.leaky"));
  }
  public static IFn foo = RT.var("leak.leaky", "foo");
}
(ns leak.leaky)

(defn foo
  "Some doc"
  []
  "hello")

(def unrelated 42)

https://gist.github.com/cgrand/5dcb6fe5b269aecc6a5b#file-main-clj-L10

Patch: 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v4.patch



 Comments   
Comment by Christophe Grand [ 18/Dec/14 3:56 PM ]

Patch from Nicola Mometto

Comment by Nicola Mometto [ 18/Dec/14 4:01 PM ]

Attached the same patch with a more informative better commit message

Comment by Laurent Petit [ 18/Dec/14 4:03 PM ]

I'd like to thank Christophe and Alex for their invaluable help in understanding what was happening, formulating the right hypothesis and then finding a fix.

I would also mention that even if non IBM rational environments where not affected by the bug to the point were CCW would not work, they were still affected. For instance the class for a one-liner function wrapping an interop call weighs 700bytes once the patch is applied, when it weighed 90kbytes with current 1.6 or 1.7.

Comment by Laurent Petit [ 18/Dec/14 5:07 PM ]

In CCW for the initial problematic function, the -v2 patch produces exactly the same bytecode as if the referenced class does not load any namespace in its static initializers.
That is, the patch is valid. I will test it live in the IBM Rational environment ASAP.

Comment by Laurent Petit [ 19/Dec/14 12:10 AM ]

I confirm the patch fixes the issue detected initially in the IBM Rational environment

Comment by Michael Blume [ 06/Jan/15 4:03 PM ]

I have absolutely no idea why, but if I apply this patch, and the patch for CLJ-1544 to master, and then try to build a war from this test project https://github.com/pdenhaan/extend-test I get a scary-looking traceback:

$ lein do clean, war!
Exception in thread "main" java.lang.NoSuchFieldError: __thunk__0__, compiling:(route.clj:1:1)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3606)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at extend_test.core.handler$loading__5301__auto____66.invoke(handler.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at extend_test.core.servlet$loading__5301__auto____7.invoke(servlet.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$compile$fn__5420.invoke(core.clj:5834)
	at clojure.core$compile.invoke(core.clj:5833)
	at user$eval5.invoke(form-init180441230737245034.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6776)
	at clojure.lang.Compiler.eval(Compiler.java:6765)
	at clojure.lang.Compiler.eval(Compiler.java:6766)
	at clojure.lang.Compiler.load(Compiler.java:7203)
	at clojure.lang.Compiler.loadFile(Compiler.java:7159)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$init_opt.invoke(main.clj:279)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.NoSuchFieldError: __thunk__0__
	at instaparse.core__init.load(Unknown Source)
	at instaparse.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at clout.core$loading__5301__auto____273.invoke(core.clj:1)
	at clout.core__init.load(Unknown Source)
	at clout.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:482)
	at compojure.core$loading__5301__auto____68.invoke(core.clj:1)
	at compojure.core__init.load(Unknown Source)
	at compojure.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at compojure.route$loading__5301__auto____1508.invoke(route.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	... 75 more
Subprocess failed
Comment by Michael Blume [ 06/Jan/15 4:06 PM ]

https://github.com/MichaelBlume/clojure/tree/no-field
https://github.com/MichaelBlume/extend-test/tree/no-field

mvn clean install in the one, lein ring uberwar in the other.

Comment by Nicola Mometto [ 06/Jan/15 6:09 PM ]

Michael, thanks for the report, I've tried investigating this a bit but the big amount of moving parts involved make it really hard to figure out why the combination of the two patches causes this issue.

A helpful minimal case would require no lein and no external dependencies, I'd appreciate some help in debugging this issue if anybody has time.

Comment by Michael Blume [ 06/Jan/15 10:56 PM ]

Ok, looks like the minimal case is

(ns foo (:require [instaparse.core]))

(ns bar (:require [foo]))

and then attempt to AOT-compile both foo and bar.

I don't yet know what's special about instaparse.core.

Comment by Michael Blume [ 06/Jan/15 11:30 PM ]

Well, not a minimal case, of course, but one without lein, at least.

Comment by Michael Blume [ 06/Jan/15 11:51 PM ]

ok, problem is instaparse's defclone macro, I've extracted it to a test repo

https://github.com/MichaelBlume/thunk-fail

lein do clean, compile will get you a failure, but the repo has no dependencies so I'm sure there's a way to do that without lein.

Comment by Ghadi Shayban [ 06/Jan/15 11:56 PM ]

Sorry for the barrage of questions, but these classloader bugs are subtle (and close to being solved I hope). Your report is immensely valuable, and yet it will help to be even more specific. There are a cluster of these bugs – and keeping them laser-focused is key.

The minimal case to which you refer is the NoSuchFieldError?
How are is this being invoked this without lein?
What are you calling to AOT? (compile 'bar) ?
What is the classpath? When you invoke originally, is ./target/classes empty?
Does the problem go away with CLJ-979-7 applied?

Comment by Michael Blume [ 07/Jan/15 12:16 AM ]

I have tried and failed to replicate without leiningen. When I just run

java -Dclojure.compile.path=target -cp src:../clojure/target/clojure-1.7.0-aot-SNAPSHOT.jar clojure.lang.Compile thunk-fail.first thunk-fail.second

everything works fine.

Comment by Ghadi Shayban [ 07/Jan/15 12:30 AM ]

The NoSuchFieldError is related to the keyword lookup sites.

Replacing defclone's body with
`(do (:foo {})) is enough to trigger it, with the same ns structure.

Comment by Nicola Mometto [ 07/Jan/15 4:47 AM ]

I have updated the patch for CLJ-1544, now the combination of the new patch + the patch from this ticket should not cause any exception.

That said, a bug in this patch still exists since while the patch for CLJ-1544 had a bug, it was causing a perfectly valid (albeit hardly reproducible) compilation scenario so we should keep debugging this patch with the help of the bugged patch for CLJ-1544.

I guess the first thing to do is figure out what lein compile is doing differently than clojure.Compile

Comment by Nicola Mometto [ 07/Jan/15 4:49 AM ]

Also Ghadi is right, infact replacing the whole body of thunk-fail.core with (:foo {}) is enough.

It would seem like the issue is with AOT (re)compiling top-level keyword lookup sites, my guess is that for some reason this patch is preventing correct generation of the __init static initializer.

Comment by Nicola Mometto [ 07/Jan/15 5:35 AM ]

I still have absolutely no idea what lein compile is doing but I figured out the issue.
The updated patch binds (in eval) the appropriate vars only when already bounded.

Comment by Alex Miller [ 07/Jan/15 9:00 AM ]

Would it be worth using transients on the bindings map now?

Comment by Nicola Mometto [ 07/Jan/15 9:11 AM ]

Makes sense, updated the patch to use a transient map

Comment by Michael Blume [ 07/Jan/15 12:25 PM ]

Is there a test we can add that'll fail in the presence of the v2 patch? preferably independent of the CLJ-1544 patch? I can try to write one myself, but I don't have a lot of familiarity with the Clojure compiler internals.

Comment by Nicola Mometto [ 07/Jan/15 12:32 PM ]

I'll have to think about a way to reproduce that bug, it's not a simple scenario to reproduce.
It involves compiling a namespace from an evaluated context.





[CLJ-1619] PersistentVector implements IReduce but the no init arity throws Created: 17/Dec/14  Updated: 10/Jan/15  Resolved: 10/Jan/15

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

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

Attachments: Text File 0001-CLJ-1619-Implement-no-init-arity-of-reduce-for-Persi.patch     Text File 0001-Implement-no-init-arity-of-reduce-for-PersistentVect.patch    
Patch: Code and Test
Approval: Ok

 Description   

The reduce arity of IReduce in PersistentVector is implemented as: "throw new UnsupportedOperationException()".

After the CLJ-1572 patch is applied the following code will throw:

(reduce + [1 2])

Approach taken: Implement reduce(f) in PersistentVector.

Alternative: An alternate would be to change PersistentVector from IReduce to IReduceInit and remove the reduce without init function. In this case, reducing a vector would fall back to seqs.

Patch: 0001-CLJ-1619-Implement-no-init-arity-of-reduce-for-Persi.patch

Screened by: Stu



 Comments   
Comment by Alex Miller [ 18/Dec/14 10:59 AM ]

Is that return null there right? In the case of no elements, you should invoke f with no args right?

Comment by Nicola Mometto [ 18/Dec/14 11:04 AM ]

you're right, I didn't know that detail about the behaviour of reduce. Updated the patch to invoke (f) rather than returning nil when the coll is empty

Comment by Alex Miller [ 09/Jan/15 8:14 AM ]

Needs tests

Comment by Nicola Mometto [ 09/Jan/15 8:33 AM ]

Updated patch adding testcases for PersistentVector.reduce in the already existing reduce test

Comment by Stuart Halloway [ 09/Jan/15 11:50 AM ]

I don't understand the problem here. coll-reduce appears to cut off ever hitting this path (the tests call underlying interfaces directly).

  • Need a public API example showing the failure
  • Need tests covering main branches (i.e. the empty case)
Comment by Stuart Halloway [ 09/Jan/15 11:52 AM ]

nevermind, screening 1572





[CLJ-1618] Widen set to take Iterable/IReduceInit Created: 17/Dec/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

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

 Description   

Similar to CLJ-1546 (same thing on vec), set should work on IReducibleInit or Iterable. Currently eduction will work via Iterable but through SeqIterator. set on an IReduceInit will throw an error.

user=> (set (eduction (map inc) (range 100)))  ;; works, but slower path
user=> (set (reify clojure.lang.IReduceInit  
       (reduce [_ f start]
         (reduce f start (range 10)))))
IllegalArgumentException Don't know how to create ISeq from: user$eval1198$reify__1199  clojure.lang.RT.seqFrom (RT.java:506)

Approach: Check for and use IReduceInit path if available, otherwise fallback to seq. Additionally, the patch adds a modification to return a set without it's meta (same approach as CLJ-1546) if a set is passed, which is fast constant time with no change in effective behavior.

Performance: (using Criterium quick-bench)

Timings done with either (count (set coll)) or (count (into #{} coll)):

coll 1.6.0 into 1.6.0 set 1.7.0-alpha4 set 1.7.0-alpha4+patch set
(set (range 100)) 15.4 µs 17.0 µs 11.4 µs 0.0 µs
(vec (range 1000000)) 360.7 ms 702.5 ms 391.1 ms 358.6 ms
(doall (range 1000000)) 363.6 ms 736.9 ms 387.5 ms 371.0 ms
(doall (range 5)) 404.9 ns 612.3 ns 481.9 ns 445.9 ns
(eduction (map identity) (vec (range 100))) n/a n/a 11.3 µs 8.7 µs

See also: CLJ-1546, CLJ-1384

Patch: clj-1618.patch

Screened by:






[CLJ-1616] Frequencies incompatible with eduction Created: 14/Dec/14  Updated: 14/Dec/14  Resolved: 14/Dec/14

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

Type: Defect Priority: Major
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: None


 Description   

Reproduction:
This needs the CLJ-1606 patch to apply, so that eduction works.

(frequencies (eduction (take 5) (range 50)))
;; ArityException Wrong number of args (1) passed to: core/frequencies/fn--6730

Cause: The reduce function that 'frequencies' calls is lacking the completing arity.

Simplest fix is to add the completing arity. Could be useful to allow frequencies to take a transducer stack.

mapv/filterv are similarly affected but seem less useful than using into with transducers.



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

Doesn't this work with CLJ-1572 + CLJ-1606 patches?

Comment by Ghadi Shayban [ 14/Dec/14 9:11 PM ]

No, not when there is something like 'take' in the picture. Transducers imply a reducing function with two different arities [1]. When 'frequencies' reduces over the collection (the eduction), a transducer inside the eduction might terminate early and cause the arity-1 rfn to be called, which will eventually bottom out here and throw the missing arity. [2]

CLJ-1572 helps dispatch properly
CLJ-1606 helps eduction actually work

[1] https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L6520-L6521
[2] https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L6859

Comment by Alex Miller [ 14/Dec/14 9:49 PM ]

The example given works for me when I have CLJ-1572 + CLJ-1606 - what am I missing?

Comment by Ghadi Shayban [ 14/Dec/14 10:42 PM ]

Sigh you're not missing anything. I have an active repl that I can reproduce this on...

But with a bare build with CLJ-1572 and CLJ-1606 applied it does not happen. Give me a little bit to track this down. Intuitively it seems correct that something trying to complete frequencies's rfn:

(fn [counts x]
             (assoc! counts x (inc (get counts x 0))))

would fail.

Comment by Ghadi Shayban [ 14/Dec/14 10:49 PM ]

I'll reopen if I can figure out what happened





[CLJ-1615] transient set "keys" and "values" wind up with different metadata Created: 12/Dec/14  Updated: 13/Dec/14

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

Type: Defect Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, meta, transient

Attachments: Text File 0001-CLJ-1615-ensure-transient-set-keys-and-values-have-c.patch     Text File 0001-demonstrate-CLJ-1615.patch     Text File CLJ-1615-entryAt.patch    
Patch: Code and Test

 Description   
(let [s (-> #{} 
          transient 
          (conj! (clojure.core/with-meta [-7] {:mynum 0}))
          (conj! (clojure.core/with-meta [-7] {:mynum -1})) 
          persistent!)]
  [(meta (s [-7])) (meta (first s))])
=> [{:mynum -1} {:mynum 0}]

basically it looks like the "key" (the value we get by seqing on the set) retains the metadata from the first conj! but the "value" (what we get by calling invoke with the "key") carries the metadata from the second conj!. This does not match the behavior if we don't use transients:

(let [s (-> #{} 
          (conj (clojure.core/with-meta [-7] {:mynum 0}))
          (conj (clojure.core/with-meta [-7] {:mynum -1})))]
  [(meta (s [-7])) (meta (first s))])
=> [{:mynum 0} {:mynum 0}]

(found playing with zach tellman's collection-check)



 Comments   
Comment by Michael Blume [ 12/Dec/14 5:07 PM ]

Attached patch demonstrating problem (not a fix)

Comment by Michael Blume [ 12/Dec/14 5:40 PM ]

More investigation:

The difference between "keys" and "vals" arises from the fact that clojure sets use maps under the covers.

The difference between persistent and transient seems to be because PersistentHashSet.cons short-circuits on contains (https://github.com/clojure/clojure/blob/clojure-1.6.0/src/jvm/clojure/lang/PersistentHashSet.java#L97) and ATransientSet.conj does not (https://github.com/clojure/clojure/blob/clojure-1.6.0/src/jvm/clojure/lang/ATransientSet.java#L27)

Adding a contains check to ATransientSet.conj makes the behavior consistent and passes the attached test, but I imagine this could cause a performance hit. Thoughts?

Comment by Michael Blume [ 12/Dec/14 5:43 PM ]

Attached proposed fix – note that this may cause a performance hit for transient sets.

Comment by Michael Blume [ 13/Dec/14 2:40 PM ]

Attaching an alternative fix – instead of doing a contains check on every transient conj, back set.get with entryAt. More invasive but possibly faster.





[CLJ-1614] Clojure does not start: ClassCastException Created: 12/Dec/14  Updated: 12/Dec/14

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

Type: Defect Priority: Minor
Reporter: Vladimir Tsichevski Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler
Environment:

Eclipse RCP



 Description   

The clojure.lang.Compiler class static code throws the ClassCastException when reading compiler options from System properties (Compiler.java, line 260 in the git master release). When running Clojure from Eclipse RCP application the System properties may have non-string values.

Checking if the value is String and ignoring non-strings fixes this problem.






[CLJ-1613] :or defaults should refer to enclosing scope in map destructuring Created: 12/Dec/14  Updated: 31/Dec/14

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

Type: Defect Priority: Major
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-CLJ-1613-evaluate-or-defaults-in-enclosing-scope-in-.patch    
Patch: Code and Test

 Description   

Michael Blume noticed that :or defaults can depend on the values of other keys, see https://groups.google.com/d/msg/clojure/6kOhpPOpHWM/ITjWwQFS_VQJ

Michael's Gist https://gist.github.com/MichaelBlume/4891dafdd31f0dcbc727 displays a case where an associative form involving :keys and :or compiles or not depending on the order of symbols in :keys. By tweaking that case one can arrive at expressions which always compile, but produce different values depending on :keys:

(let [foo 1
       bar 2
       {:keys [bar foo]
        :or {foo 3 bar (inc foo)}} {}]
  {:foo foo :bar bar})
;= {:foo 3, :bar 4}

(let [foo 1
      bar 2
      {:keys [foo bar]
       :or {foo 3 bar (inc foo)}} {}]
  {:foo foo :bar bar})
;= {:foo 3, :bar 2}

I believe that the most natural solution is to demand that :or defaults be evaluated in an enclosing scope where none of the destructuring-introduced locals are present. This approach is taken by the 0001 patch.



 Comments   
Comment by Michael Blume [ 12/Dec/14 2:27 AM ]

I suspect that this is the right thing to do but I think it's important to note that this will break existing code https://github.com/ngrunwald/ring-middleware-format/blob/master/src/ring/middleware/format_params.clj#L214





[CLJ-1612] clojure.core.reducers/mapcat can call f1 with undefined arity of 0 arguments? Created: 10/Dec/14  Updated: 10/Dec/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: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers


 Description   

I have not run across this with running code, so perhaps it is impossible for reasons I have not understood. Also not sure whether fixing issues with reducers is of any importance, given transducers. This was found while testing the Eastwood lint tool on some Clojure namespaces, including clojure.core.reducers.

(defcurried mapcat
  "Applies f to every value in the reduction of coll, concatenating the result
  colls of (f val). Foldable."
  {:added "1.5"}
  [f coll]
  (folder coll
   (fn [f1]
     (let [f1 (fn
                ([ret v]
                  (let [x (f1 ret v)] (if (reduced? x) (reduced x) x)))
                ([ret k v]
                  (let [x (f1 ret k v)] (if (reduced? x) (reduced x) x))))]
       (rfn [f1 k]
            ([ret k v]
               (reduce f1 ret (f k v))))))))

The definition of macro rfn expands to a (fn ...) that can call f1 with no arguments, which is not a defined arity for f1.






[CLJ-1611] clojure.java.io/pushback-reader Created: 08/Dec/14  Updated: 11/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: io, reader

Attachments: Text File drupp-clj-1611-2.patch     Text File drupp-clj-1611.patch    
Approval: Triaged

 Description   

Whereas

  • clojure.core/read and clojure.edn/read require a PushbackReader;
  • clojure.java.io/reader produces a BufferedReader, which isn't compatible;
  • the hazard has tripped folks up for years[1];
  • clojure.java.io is pure sugar anyway (and would not be damaged by the addition of a little bit more);
  • clojure.java.io's very existence suggests suitability and fitness for use (wherein by the absence of a read-compatible pushback-reader it falls short);

i.e., in the total absence of clojure.java.io it would not seem "hard" to use clojure.edn, but in the presence of clojure.java.io and its "reader" function, amidst so much else in the API that does fit together, one keeps thinking one is doing it wrong;

and

  • revising the "read" functions to make their own Pushback was considered but rejected [2];

Therefore let it be suggested to add clojure.java.io/pushback-reader, returning something consumable by clojure.core/read and clojure.edn/read.

[1] The matter was discussed on Google Groups:

(2014, "clojure.edn won't accept clojure.java.io/reader?") https://groups.google.com/forum/#!topic/clojure/3HSoA12v5nc

with a reference to an earlier thread

(2009, "Reading... from a reader") https://groups.google.com/forum/#!topic/clojure/_tuypjr2M_A

[2] CLJ-82 and the 2009 message thread



 Comments   
Comment by David Rupp [ 10/Jan/15 4:05 PM ]

Attached patch drupp-clj-1611.patch implements clojure.java.io/pushback-reader as requested.

Comment by David Rupp [ 10/Jan/15 4:07 PM ]

Note that you can always import java.io.PushbackReader and do something like (PushbackReader. (reader my-thing)) yourself; that's really all the patch does.

Comment by Phill Wolf [ 11/Jan/15 7:54 AM ]

clojure.java.io/reader is idempotent, while the patch of 10-Jan-2015 re-wraps an existing PushbackReader twice: first with a new BufferedReader, then with a new PushbackReader.

Leaving a given PushbackReader alone would be more in keeping with the pattern of clojure.java.io.

It also needs a docstring. If pushback-reader were idempotent, the docstring's opening phrase could echo clojure.java.io/reader's, e.g.: Attempts to coerce its argument to java.io.PushbackReader; failing that, (bla bla bla).

Comment by David Rupp [ 11/Jan/15 11:14 AM ]

Adding drupp-clj-1611-2.patch to address previous comments.





[CLJ-1610] Unrolled small maps Created: 08/Dec/14  Updated: 08/Dec/14

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Zach Tellman
Resolution: Unresolved Votes: 1
Labels: collections

Approval: Vetted

 Description   

Placeholder for unrolled small maps enhancement (companion for vectors at CLJ-1517).






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

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

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

Attachments: Text File reflector_method_bug.patch    
Patch: Code
Approval: Triaged

 Description   

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

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

I've outlined a small example here: https://github.com/jeremyheiler/clj-reflector-bug

The repo contains a Java example that works, and the same example in Clojure that doesn't work. It also includes a patched version of Clojure 1.6.0, and shows that the patch solves the issue. Also, in `src/foo/m.clj`, there is a real example of this bug occurring by using the Java Debug Interface API in the tools.jar library.

I would have added tests to the patch, but I don't think the test runner compiles test Java code, which is required.



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

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

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

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

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

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





[CLJ-1608] add split-at to clojure.string Created: 03/Dec/14  Updated: 03/Dec/14  Resolved: 03/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Dmitr Sotnikov Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: string

Attachments: Text File string.clj.patch    
Patch: Code

 Description   

Add clojure.string/split-at similar to clojure.core/split-at that accepts a string and a number indicating the position where the string should be split. The function returns a vector containing two strings, first containing the characters from 0-n-1, and second n-length.



 Comments   
Comment by Alex Miller [ 03/Dec/14 9:48 PM ]

I do not think this is an operation that is fundamental (it can be easily composed from existing functions like count and subs) or represents a portability opportunity by being a function available on jvm and js with host performance benefits. It is a non-goal for clojure.string to contain every potentially useful string function.

Comment by Dmitr Sotnikov [ 03/Dec/14 11:04 PM ]

Makes sense, thanks for the clarification.





[CLJ-1607] docstring for clojure.core/counted? should be more specific Created: 29/Nov/14  Updated: 01/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: docstring

Attachments: Text File CLJ-1607-p1.patch    
Approval: Triaged

 Description   

The docstring for counted? currently says:

Returns true if coll implements count in constant time

This tempts the user into thinking they can use this function to determine whether or not calling count on any collection is a constant-time operation, when in fact it only reflects whether or not an object implements the clojure.lang.Counted interface. Since count special-cases a handful of platform types, there are common cases such as Arrays and Strings that are constant time but will return false from counted?.

Proposed:

Returns true if coll, a Clojure collection, implements count in constant time. Note that this function will return false for host types even if the count function can return their size in constant time (as with arrays and strings).



 Comments   
Comment by Gary Fredericks [ 29/Nov/14 9:01 AM ]

Attached CLJ-1607-p1.patch with my first draft of a better docstring.

Comment by Gary Fredericks [ 29/Nov/14 9:08 AM ]

What would be the most accurate language to describe the exceptions? I used "some collections" in the first patch but perhaps "native collections" or "host collections" would be more helpful?

Comment by Alex Miller [ 29/Nov/14 9:44 AM ]

While I understand where you're coming from, I think the intent of "counted?" is not to answer the question "is this thing countable in constant time" for all possible types, but specifically for collections that participate in the Clojure collection library. This includes both internal collections like PHM, PHS, PV, etc but also external collections that mark their capabilities using those interfaces.

I believe count handles more cases than just collections that are counted in constant time (like seqs) so is not intended to be symmetric with counted?.

Comment by Gary Fredericks [ 29/Nov/14 9:55 AM ]

Sure, I wasn't suggesting changing what the function does – just changing the docstring to make it less likely to be misleading.

Comment by Gary Fredericks [ 29/Nov/14 10:00 AM ]

What about this sort of wording?

Returns true if coll, a Clojure collection, implements count in constant time.
Note that this function will return false for host types even if the count 
function can return their size in constant time (as with arrays and strings).
Comment by Alex Miller [ 30/Nov/14 9:52 PM ]

I think it's unlikely to pass vetting, but that's just my guess.

Comment by Gary Fredericks [ 01/Dec/14 8:53 AM ]

I'm trying to figure out where the disagreement is here; are you arguing any of these points, or something different?

  1. The docstring is not likely to confuse people by making them think it gives meaningful responses for host collections
  2. It's not a problem for us to solve if the docstring confuses people
  3. It is a problem we should solve, but the changes I've suggested are a bad solution
Comment by Alex Miller [ 01/Dec/14 9:18 AM ]

In general, the docstrings prefer concision and essence over exhaustive cases or examples. My suspicion is that the docstring says what Rich wants it to say and he would consider the points you've added to be implicit in the current docstring, and thus unnecessary. Specifically, "coll" is used pretty consistently to mean a Clojure collection (or sequence) across all of the docstrings. And there is an implicit else in the docstring that counted? will return false for things that are not Clojure collections. The words that are there (and not there) are carefully chosen.

I agree with you that more words may be necessary to describe fully what to expect from this or any other function in core. My experience from seeing Rich's response on things like this is that he may agree with that too, but he would prefer it to live somewhere outside the doc string in reference material or other sources. Not to say that we don't update docstrings, as that does happen pretty regularly; I just don't think this one will be accepted. I've asked Stu to give me a second set of eyes too.

Comment by Gary Fredericks [ 01/Dec/14 9:36 AM ]

That was helpful detail, thanks!

Comment by Reid McKenzie [ 01/Dec/14 12:42 PM ]

I think this one is fine as-is, because the docstring for count explicitly notes "Also works on ..." which are implied not to be counted?.





[CLJ-1606] Transducing an eduction finishes twice Created: 27/Nov/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Defect Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: transducers
Environment:

1.7.0-alpha4


Attachments: Text File CLJ-1606-2.patch     Text File CLJ-1606-3.patch     Text File CLJ-1606-4.patch     Text File CLJ-1606-5.patch     Text File CLJ-1606.patch    
Patch: Code and Test
Approval: Ok

 Description   
> (transduce (map identity)
             (fn
               ([s] (println "Finishing") s)
               ([s i] s))
             nil
             (eduction (map identity) []))
Finishing
Finishing
nil

Cause: transduce passes (xf f) into .reduce of Eduction, which calls transduce, causing completing xf to be called more than once.

Proposed: Eduction reduce should use (completing f) instead of f to isolate completion of inner xf from outer xf.

Patch: CLJ-1606-5.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 27/Nov/14 11:01 PM ]

identity is not a valid xf - changed to (map identity)

Comment by Ghadi Shayban [ 27/Nov/14 11:34 PM ]

identity is a valid though nonsensical transducer. fix & test added.

Comment by Ghadi Shayban [ 28/Nov/14 12:06 AM ]

Simple reproduction similar to into:

(transduce (map dec)
           (completing conj! persistent!)
           (transient [])
           (eduction (map inc) (range 6)))

;; ClassCastException clojure.lang.PersistentVector cannot be cast to clojure.lang.ITransientCollection

into doesn't use completing, and conj! has an arity that hides the problem.

Comment by Alex Miller [ 28/Nov/14 8:54 AM ]

I removed trailing whitespace in the patch so it applies cleanly.

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

This patch is a little more subtle than I thought. Completion of the eduction's rfn needs to be handled separately from the "outer" transduce's xform. Patch coming.

Comment by Ghadi Shayban [ 14/Dec/14 11:32 PM ]

New patch with tests that completes the inner xform without completing the passed in rfn

Comment by Ghadi Shayban [ 15/Dec/14 1:19 AM ]

both -3 and -2 are equivalent. -3 is probably better stylistically.

Comment by Alex Miller [ 15/Dec/14 8:37 AM ]

Added CLJ-1606-4.patch - identical to -3, just fixed whitespace error.

Comment by Andy Fingerhut [ 08/Jan/15 6:09 PM ]

There are two identically named attachments here (containing -2). It looks like it isn't the one under consideration, but it might be nice to remove or rename to avoid the name conflict.

Comment by Ghadi Shayban [ 08/Jan/15 6:24 PM ]

Andy, not sure how to do that, but in any case I just added -5 clarifying language in the comment

Comment by Alex Miller [ 08/Jan/15 6:41 PM ]

Ghadi, that was super confusing. Did you just add a new -5 patch? The -4 patch has already been screened, and you have not removed the duplicate -2 patch so I don't get what the -5 is. Can we just delete the -5 and older -2 patches?

Comment by Andy Fingerhut [ 08/Jan/15 6:44 PM ]

Sorry for adding to the confusion. Ghadi, instructions for deleting patches are in the "Removing patches" section on this wiki page: http://dev.clojure.org/display/community/Developing+Patches

Comment by Ghadi Shayban [ 08/Jan/15 6:50 PM ]

Sorry. Fine by me, though permissions prevent me from deleting one of the patches.

As I read through the screened patch I just tried to clarify the wording. This:

;; NB (completing f) isolates completion of inner xfns from outer xfns
became:
;; NB (completing f) isolates completion of inner rf from outer rf

Feel free to nix that -5 patch if that's worthless

Comment by Alex Miller [ 08/Jan/15 7:12 PM ]

Gotcha. I will take care of the further changes later tonight.

In the future, please don't modify screened patches without letting me know.





[CLJ-1605] Unexpected additional digits are appeared after RuntimeException in repl. Created: 26/Nov/14  Updated: 27/Nov/14  Resolved: 26/Nov/14

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

Type: Defect Priority: Trivial
Reporter: Constantine Potapov Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: bug
Environment:

$uname -a
Linux um 3.13.0-24-generic #47-Ubuntu SMP Fri May 2 23:30:00 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux

$ java -version
java version "1.7.0_25"
Java(TM) SE Runtime Environment (build 1.7.0_25-b15)
Java HotSpot(TM) 64-Bit Server VM (build 23.25-b01, mixed mode)



 Description   

1) run repl
lein repl

  • evaluate the following value
    ( + (/ 2 3) (/ 3 4 ) )
    17/12
    • result is correct

2) evaluate it with an error, the space after "/" was deleted (/2 3)
user=> ( + (/2 3) (/ 3 4 ))

RuntimeException Invalid token: /2 clojure.lang.Util.runtimeException (Util.java:221)
RuntimeException Unmatched delimiter: ) clojure.lang.Util.runtimeException (Util.java:221)
RuntimeException Unmatched delimiter: ) clojure.lang.Util.runtimeException (Util.java:221)

  • try to evaluate it again
    user=> ( + (/ 2 3) (/ 3 4 ) )
    33/417/12
    • result is incorrect
      actual value is: "33/417/12"
      the value was expected: "17/12"
  • if it runs second time, result is correct again
    user=> ( + (/ 2 3) (/ 3 4 ) )
    17/12


 Comments   
Comment by Nicola Mometto [ 26/Nov/14 6:32 AM ]

The problem is that `/2` is not a valid clojure expression, an error is thrown and 3 is returned, then (/ 3 4) is evaluated and 3/4 is returned.
It looks there's an issue with lein repl that causes "3", "3/4" to be printed after the next expression is evaluated, that is "( + (/ 2 3) (/ 3 4 ) )" which returns 17/12.

This is why you get the apparently wrong result "33/417/12", when in fact it is just printing 3 results in the same line: 3 3/4 17/12

In short, this is not a bug in clojure, it's a bug in how lein repl prints the result. you should open a ticket in the lein issue tracker https://github.com/technomancy/leiningen

Comment by Nicola Mometto [ 26/Nov/14 6:33 AM ]

For instance, here's how a repl run using java -jar clojure.jar prints it:

user=> ( + (/2 3) (/ 3 4 )) 
RuntimeException Invalid token: /2  clojure.lang.Util.runtimeException (Util.java:221)
3
RuntimeException Unmatched delimiter: )  clojure.lang.Util.runtimeException (Util.java:221)
3/4
RuntimeException Unmatched delimiter: )  clojure.lang.Util.runtimeException (Util.java:221)
user=> ( + (/ 2 3) (/ 3 4 ) )
17/12
Comment by Alex Miller [ 26/Nov/14 11:39 AM ]

see Nicola's comment - behavior change is an issue with lein repl

Comment by Constantine Potapov [ 27/Nov/14 2:28 AM ]

I have opened the bug for the lein repl here https://github.com/technomancy/leiningen/issues/1774





[CLJ-1604] AOT'ed code that defs a var with clojure.core symbol name causes IllegalStateException Created: 25/Nov/14  Updated: 21/Feb/15  Resolved: 20/Feb/15

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

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

Attachments: Text File 0001-fix-AOT-bug-preventing-overriding-of-clojure.core-fu.patch     Text File 0001-fix-AOT-bug-preventing-overriding-of-clojure.core-fu-v2.patch     File 1604-context.diff    
Patch: Code
Approval: Ok

 Description   

AOT'ed code that defs a var that is also a symbol in clojure.core results in an exception at runtime. This problem can be avoided with (:refer-clojure :exclude ...) but this requires a library author to update and release a new version. AOT'ed applications must then wait for all transitive dependencies to update before they can update to a new Clojure version. For some users, this problem prevents them from trying or adopting new releases.

For example, the contrib library data.int-map defines an update function. clojure.core will also have a new update function as of 1.7.0. If this library is AOT'ed, then users of the clojure.data.int-map/update function will see the exception below. This situation can commonly occur when an application uses lein uberjar to compile all of the project+libs. In this case, applications or libraries that use data.int-map (either directly or indirectly) are affected.

java.lang.IllegalStateException: Attempting to call unbound fn: #'clojure.data.int-map/update
 at clojure.lang.Var$Unbound.throwArity (Var.java:43)
    clojure.lang.AFn.invoke (AFn.java:40)
    compiler_update_not_referenced_bug.core$foo.invoke (core.clj:5)

Reproduce with this sample project: https://github.com/yeller/compiler_update_not_referenced_bug

Cause: When AOT compiling a namespace, the def forms are hoisted into the ns__init class (in the example here, clojure.data.int_map__init). The static initializer in this class creates each var in the ns via a call to RT.var(ns, name). For data.int-map the static initializer will properly create the var for clojure.data.int-map/update. But when the ns is loaded (via the clojure.data.int_map.load() method), (refer-clojure) will be called, which will remap clojure.data.int-map/update to point to clojure.core/update.

This problem does not affect non-AOT loading (which doesn't use the ns__init class) and does not affect collisions from any other namespace. Only collisions from clojure.core create this possibility.

Proposed: The proposed patch explicitly refers the Var during ns__init.load() (after Clojure symbols are referred) rather than implicitly during ns__init static {}.

This change in behavior only happens during AOT in the specific case where a core symbol is being shadowed. In that case, clojure.core has already been loaded and v (the looked up var) will have ns=clojure.core. The currentNS will be (for example) data.int-map. If that's the case, and the sym has no ns, then the new logic will be emitted.

In the case of clojure.core itself, NO new bytecode is emitted. From tests on several projects, only shadowed vars during AOT get this additional bytecode.

Patch: 0001-fix-AOT-bug-preventing-overriding-of-clojure.core-fu-v2.patch

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 25/Nov/14 11:28 PM ]

When I try latest Clojure master plus patch CLJ-1604-only-core.patch with the small test project created by Tom Crayford to demonstrate this issue: https://github.com/yeller/compiler_update_not_referenced_bug

In that project, I get the same exception thrown when attempting 'lein do clean, uberjar, test' using this patch, as without it. It is because int-map/update in namespace compiler-update-not-referenced-bug.core is an unbound var.

Comment by Nicola Mometto [ 26/Nov/14 4:25 AM ]

Andy, you're right. For some reason I attached the wrong patch to the ticket, this is the correct one

Comment by Nicola Mometto [ 26/Nov/14 5:21 AM ]

I wasn't able to write a test for this, so here's a repl session using the clojure jar demonstrating this issue:

[˷/test]> ls
classes  clojure.jar  test.clj
[˷/test]> cat test.clj
(in-ns 'test)
(clojure.core/refer 'clojure.core)
(def foo "bar")
(def update "foo")
[˷/test]> java -cp classes:clojure.jar:. clojure.main
Clojure 1.7.0-master-SNAPSHOT
user=> (binding [*compile-files* true] (load "test"))
WARNING: update already refers to: #'clojure.core/update in namespace: test, being replaced by: #'test/update
nil
user=> test/foo
"bar"
user=> test/update
"foo"
user=>
[˷/test]> java -cp classes:clojure.jar:. clojure.main
Clojure 1.7.0-master-SNAPSHOT
user=> (load "test")
nil
user=> test/foo
"bar"
user=> test/update
CompilerException java.lang.RuntimeException: No such var: test/update, compiling: (NO_SOURCE_PATH:0:0)
user=>
Comment by Andy Fingerhut [ 26/Nov/14 10:39 AM ]

Thanks. I have not tried to assess the details of the change, other than to say that patch 0001-fix-AOT-bug-preventing-overriding-of-clojure.core-fu.patch dated 26 Nov 2014, when applied to latest Clojure master as of today, enables both 'lein do clean, test' and 'lein do clean, uberjar, test' to work as expected with Tom Crayford's test project, linked above, whereas 'lein do clean, uberjar, test' fails without this patch, due to a var being unbound that should have a value.

Comment by Andy Fingerhut [ 27/Nov/14 10:53 AM ]

Copying a comment here from CLJ-1591, since it is more appropriate here. It is responding to Tom Crayford's posting of his example project to demonstrate the issue: https://github.com/yeller/compiler_update_not_referenced_bug

Tom, looked at your project. Thanks for that. It appears not to have anything like (def inc inc) in it. It throws exception during test step of 'lein do clean, uberjar, test' consistently for me, too, but compiles with only warnings and passes tests with 'lein do clean, test'. I have more test results showing in which Clojure versions these results change. To summarize, the changes to Clojure that appear to make the biggest difference in the results are below (these should be added to the new ticket you create – you are welcome to do so):

Clojure 1.6.0, 1.7.0-alpha1, and later changes up through the commit with description "CLJ-1378: Allows FnExpr to override its reported class with a type hint": No errors or warnings for either lein command above.

Next commit with description "Add clojure.core/update, like update-in but takes a single key" that adds clojure.core/update: 'lein do clean, test' is fine, but 'lein do clean, uberjar' throws exception during compilation, probably due to CLJ-1241.

Next commit with description "fix CLJ-1241": 'lein do clean, test' and 'lein do clean, uberjar' give warnings about clojure.core/update, but no errors or exceptions. 'lein do clean, uberjar, test' throws exception during test step that is same as the one I see with Clojure 1.7.0-alpha4. Debug prints of values of clojure.core/update and int-map/update (in data.int-map and in Tom's namespace compiler-update-not-referenced-bug.core) show things look fine when printed inside data.int-map, and in Tom's namespace when not doing the uberjar, but when doing the uberjar, test, int-map/update is unbound in Tom's namespace.

In case it makes a difference, my testing was done with Mac OS X 10.9.5, Leiningen 2.5.0 on Java 1.7.0_45 Java HotSpot(TM) 64-Bit Server VM

Comment by Nicola Mometto [ 02/Dec/14 9:04 AM ]

The updated patch only emits the interning bytecode when necessary, avoiding the emission when a clojure.core var with the same name exists but is not mapped to the current namespace

Comment by Alex Miller [ 09/Jan/15 9:27 AM ]

Attached 1604-context.diff for purely informational purposes - same diff just more context in it for easier reading.

Comment by Tom Crayford [ 10/Jan/15 4:52 PM ]

Thought I'd add a minor note in here to say I tried testing this patch out on my app (which is where I discovered this AOT bug), and the bug doesn't turn up with this patch applied to clojure (tested by applying 0001-fix-AOT-bug-preventing-overriding-of-clojure.core-fu-v2.patch to 1.7-alpha5)

Comment by Adam Krieg [ 21/Feb/15 12:28 PM ]

I ran into this issue with Korma 0.4.0. I'm still running into it, but there is a twist.

My project depends on an artifact that was built with Clojure 1.7.0-alpha1. If I remove this dependency, everything is fine. However, with this dependency, I run into this issue, even if I declare a dependency on 1.7.0-master-SNAPSHOT in my project and exclude any dependency on clojure-1.7.0-alpha1.

I'm not sure if this is a Maven issue or a Clojure issue. Running Maven with debug on seems to show that it's using the correct version of Clojure.

I have created a dummy project that reproduces this issue if you are interested.

https://github.com/deaddowney/UpdateProblem

Check it out, run "mvn install", and you will get
java.lang.RuntimeException: No such var: korma.core/update
.





[CLJ-1603] cycle, iterate, repeat return vals should IReduceInit Created: 25/Nov/14  Updated: 02/Mar/15

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

Alternatives:

There were a number of possible approaches for these enhancements:
1) Straight Java impl. The benefit of this approach is improving the performance of both the seq and reduce paths at the expense of writing a bunch of Java. See clj-1603-3.patch for the best of these impls.

2) Clojure deftype. This required moving cycle and iterate and providing a repeat1 implementation until deftype is defined (similar to the approach with reduce1). The deftype version returns a Seqable, IReduce object and has effectively the same former implementation for seq with a new fast implementation for IReduce. This makes reduce paths fast, but leaves seq paths about the same, with the benefit of no new Java code. The downside was that the new seqs did not implement the full range of interfaces from prior impls, which could potentially break code. See clj-1603-12.patch for a patch that covers this.

3) Add Iterable or IReduceInit directly to LazySeq. Conceptually, this does not make sense for general lazy seqs. Seqs materialize and cache each value once and doing this along with the ability to iterate/reduce introduces issues with caching (might as well use seqs for that) and synchronization. However, doing this for just these specific lazy seqs is possible - see latest patch.

Approach: Latest patch makes LazySeq implement IReduce and internal macro reducible-lazy-seq lets callers supply a function to implement the reduce path.

A few things to note:

  • Added some example-based tests for iterate, cycle, and repeat where I thought they were needed. Did not add generative tests - not clear to me what these would be that would actually be valuable. All of these functions are pretty simple and the examples cover the special cases.

Performance:

Some example timing, all in µs:

Expression 1.6.0 1.7.0-alpha5 alpha5 + clj-1603-3 (Java) alpha5 + clj-1603-12 (deftype) alpha5 + clj-1603-14 (split impl)
(doall (repeat 1000 1)) 87 94 8 92 91
(into [] (repeat 1000 1)) 99 110 12 12 14
(reduce + 0 (repeat 1000 1)) 99 126 17 19 22
(into [] (take 1000) (repeat 1)) n/a 67 35 29 27
(doall (take 1000 (cycle [1 2 3]))) 101 106 78 106 111
(into [] (take 1000) (cycle [1 2 3])) n/a 73 39 45 43
(doall (take 1000 (iterate inc 0))) 93 98 71 102 112
(into [] (take 1000) (iterate inc 0)) n/a 85 39 39 38
  • clj-1603-3 is a Java class implementation - generally it's faster for both seqs and reduce (at the cost of more Java)
  • clj-1603-12 is a deftype implementation - generally it's about the same on seqs but faster on reduce
  • clj-1603-14 is the LazySeq extension to IReduceInit - generally it's good but the iterate seq path introduces an extra lazy seq to force so it's slightly slower on that path.

Patch: clj-1603-14.patch

Screened by:



 Comments   
Comment by Ghadi Shayban [ 25/Nov/14 11:01 AM ]

Stu, do you intend these to be in Java or Clojure? It could be trickier to implement in Clojure directly, as loading would have to be deferred until core_deftype loads. It's certainly tractable without breaking any backwards compatibility, and I've explored this while experimenting with Range as a deftype https://github.com/ghadishayban/clojure/commit/906cd6ed4d7c624dc4e553373dabfd57550eeff2

A macro to help with Seq&List participation could be certainly useful, as efficiently being both a Seq/List and IReduceInit isn't a party.

May be useful to list requirements for protocol/iface participation.

It seems like 'repeatedly' is another missing link in the IReduceInit story.

Rich mentioned the future integration of reduce-kv at the conj, it would also be useful to know how that could fit in.

---- Other concerns and ops that may belong better on the mailing list ----

In experimenting with more reducible sources, I put out a tiny repo (github.com/ghadishayban/reducers) a couple weeks ago that includes some sources and operations. The sources were CollReduce and not ISeq.

Relatedly, caching the hashcode as a Java `transient` field is not supported when implementing a collection using deftype (patch w/ test in CLJ-1573).

Sources:
Iterate was one of them https://github.com/ghadishayban/reducers/blob/master/src/ghadi/reducers.clj#L43-L51
Repeatedly https://github.com/ghadishayban/reducers/blob/master/src/ghadi/reducers.clj#L43-L51

Reduce/transduce-based Operations that accept transducers:
some, any, yield-first https://github.com/ghadishayban/reducers/blob/master/src/ghadi/reducers.clj#L52-L80
(any could use a better name, equiv to (first (filter...)))
some and any have a symmetry like filter/remove.

Novelty maybe for 1.8:
A transducible context for Iterables similar to LazyTransformer:
https://github.com/ghadishayban/reducers/blob/master/src/ghadi/reducers.clj#L157-L161

The unless-reduced macro was very useful in implementing the collections:
https://github.com/ghadishayban/reducers/blob/master/src/ghadi/reducers.clj#L7-L15
It is different than the ensure-reduced and unreduced functions in core.

Comment by Alex Miller [ 25/Nov/14 12:01 PM ]

When we discussed this in the past, it was in the vein of reusing some of the range work (in Java) to implement cycle and iterate (per CLJ-1515).

Comment by Ghadi Shayban [ 25/Nov/14 9:20 PM ]

Never mind about 'repeatedly'. Being both ISeq and IReduceInit for repeatedly doesn't make sense for something that relies on side-effects. Current users of repeatedly can reduce over it many times and only realize the elements once.

Comment by Alex Miller [ 05/Dec/14 11:17 PM ]

attached wip Java impl and posted some example timings

Comment by Ghadi Shayban [ 11/Dec/14 4:35 PM ]

NB iterate in this patch does not cache the realized ISeq, but recalcs it at every call to realize the tail. This is not a change in the promised behavior (docstring says "f must be side-effect free") but an implementation change, as worth noting in the changelog.

Comment by Stuart Halloway [ 02/Jan/15 1:32 PM ]

It looks like all the reduce with no inital value paths are still seq-y, and slower, as shown by e.g.

(dotimes [_ 10]
  (time
   (reduce
    +
    (repeat 10000000 1))))

(dotimes [_ 20]
  (time
   (reduce
    +
    0
    (repeat 10000000 1))))
Comment by Alex Miller [ 02/Jan/15 2:01 PM ]

On that example in master before / after patch I see:

before:

  • no init = 844 ms
  • with init = 920 ms

after:

  • no init = 124 ms
  • with init = 90 ms

Is that similar to what you see or not?

Comment by Alex Miller [ 02/Jan/15 4:21 PM ]

The clj-1603-3.patch has been updated to use effectively the same algorithm for both versions of reduce. With the -3 patch, I got ~96 ms on both examples in the prior comment. I re-ran the tests in the description and updated those as well (about the same as expected).

Comment by Stuart Halloway [ 16/Jan/15 1:18 PM ]

The tests do not seem to hit the unseeded reduce branches – do we even want these branches? The original ticket was for IReduceInit.

Comment by Michael Blume [ 18/Jan/15 1:48 PM ]

Probably worth noting – Git will happily apply the latest patch for CLJ-1603 on top of the latest patch for CLJ-1515, but the result does not compile because 1515 uses iterate and 1603 moves the definition of iterate lower in clojure.core. Not sure if this is worth fixing now or just noting for when they're actually applied.

Comment by Michael Blume [ 18/Jan/15 1:52 PM ]

Actually, here, this just moves the declare statement further up the file.

Comment by Michael Blume [ 18/Jan/15 2:19 PM ]

OK, no, the two patches are still incompatible even with the declaration order fixed:

[java] ERROR in (test-range) (LongRange.java:95)
     [java] expected: (= (take 3 (range 3 9 0)) (quote (3 3 3)))
     [java]   actual: java.lang.ClassCastException: clojure.core.InfiniteRepeat cannot be cast to clojure.lang.ISeq
     [java]  at clojure.lang.LongRange.create (LongRange.java:95)
Comment by Alex Miller [ 18/Jan/15 2:31 PM ]

The 1515 patch is actually being reworked right now - we will patch things up at application time if needed.

Comment by Alex Miller [ 19/Jan/15 10:12 AM ]

Removing screened marking so can be re-screened. Added new -7 patch that handles print-method, print-dup, and unmapping the deftype constructors so they're not visible. Thanks to Ghadi in CLJ-1515 for the idea on those.

Comment by Ghadi Shayban [ 20/Jan/15 8:08 AM ]

Review of -7 patch:

Seqable/seq implementations that return a separate ISeq like these do should forward a call to seq on the result, like eduction does. [1] (This is not necessary in these particular impls, as the LazySeqs returned are themselves ISeqs. Also because Cycle's deftype is only constructed for non-empty cycles, the fact that there is a guaranteed seq is implicit. Probably a best practice to add an innocuous seq call if users look to these impls as a recipe.)

The performance regression in (doall (repeat 1000 1)) should go away completely with the dorun tweak in CLJ-1515. This is because dorun is effectively calling seq twice (it calls seq, throws away the result, then calls next.)

minor nits
1) repeat1 seems to be identical to repeat-seq and has both arities necessary for the deftypes
2) inside FiniteRepeat s/(> i 0)/(pos? i) also inside the repeat constructor
3) some things are defn- , some are ^:private
4) Cycle/reduce the recur binding can be (recur rr (or (next s) coll)) rather than nil? check

[1] https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L7324

Comment by Alex Miller [ 22/Jan/15 10:34 PM ]

Ghadi - good comments! Fixed 1,2,4. #3 - ^:private is because defn- is not yet defined. New -8 patch.

Comment by Alex Miller [ 23/Jan/15 10:03 AM ]

Bah.

user=> (= (repeat 5 :a) (repeat 5 :a))
false
Comment by Alex Miller [ 23/Jan/15 3:04 PM ]

Updated to -9 patch that handles hash and equality for finite repeat case.

Comment by Ghadi Shayban [ 26/Jan/15 2:24 PM ]

metadata in the wrong place on #'repeat1

Comment by Alex Miller [ 26/Jan/15 3:27 PM ]

Thanks, fixed in -10.

Comment by Stuart Halloway [ 20/Feb/15 10:19 AM ]

The collections returned by these APIs promise several things the new deftypes do not deliver. For example, 1.6's repeat currently has the following ancestors that are lost in the propopsed deftype:

  • clojure.lang.ISeq
  • java.io.Serializable
  • clojure.lang.IPending
  • java.util.Collection
  • java.util.List
  • java.lang.Iterable
  • clojure.lang.Obj
  • clojure.lang.IPersistentCollection
  • clojure.lang.IMeta
  • clojure.lang.IObj

Losing metadata and serializability are certainly regressions, other stuff maybe as well. I suspect similar problems in all the other API returns.

We could improve testing by taking advantage of the property-checking fns already in test-clojure/data-structures, e.g. is-same-collection

Comment by Alex Miller [ 20/Feb/15 10:42 AM ]

I think we should consider carefully what is contractually required by the return of repeat. My opinion is that repeat must return something seqable, not literally a seq (or lazy seq). With that perspective, ISeq, IPending (there re lazy seq laziness), Collection, List, Iterable, and IPersistentCollection are non-essential. Obj is a concrete class and we shouldn't commit to that.

  • IObj is a gap, this should work but doesn't: (with-meta (repeat 1 :a) {:a 1})
  • IMeta doesn't need to be added, will never have meta right now and meta handles this correctly
  • Serializable - doesn't make sense for the infinite seqs but should probably fix for finite range
Comment by Alex Miller [ 20/Feb/15 11:03 AM ]

Added -11 patch that adds IObj for all the new deftypes and Serializable for FiniteRepeat. Still need to add more tests.

Comment by Stuart Halloway [ 20/Feb/15 12:27 PM ]

I think all the java. interfaces are mandatory for interop. Leaving out any one of the clojure. interfaces creates observable change in behavior composing with other core fns (admittedly the IPending case would be bizarre, who uses that?), so those seem mandatory too.

Agreed Obj should be irrelevant, and if it isn't the bug is elsewhere.

Comment by Alex Miller [ 27/Feb/15 9:41 AM ]

pending further discussion w/ stu





[CLJ-1602] vals and keys return values should implement IReduceInit or Iterable Created: 25/Nov/14  Updated: 15/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File clj-1602-2.diff     File clj-1602-3.diff     File clj-1602-4.diff     File clj-1602.diff    
Patch: Code and Test
Approval: Vetted

 Description   
  • with generative tests
  • with perf demos

Background: clojure.core/keys calls RT.keys(Object) calls APersistentMap.KeySeq.create(ISeq). RT.keys() creates a sequence (of Map.Entry objects) and KeySeq just wraps it, calling .keyValue(). There is an equivalent vals -> RT.vals() -> ValSeq path. Both of these seq impls extend ASeq and provide Iterable implementations via SeqIterator (iterator wrapped over the seq).

Approach: The important thing here is to avoid creating the sequence and instead directly iterate/reduce over the map. Noting that CLJ-1499 provides support for making PHM directly Iterable and that KeySeq/ValSeq already implement Iterable, I chose to focus on making the instances returned from keys and vals support Iterable directly on the underlying map instead of via the seq.

RT.keys()/vals() created the seq and passed it to KeySeq/ValSeq which made it too late to directly cover the original map iterator. There are a few places that rely on passing a seq of Map.Entry to keys/vals (not just a map instance), so I check for IPersistentMap and in that case pass it directly to a new KeySeq factory method that remembers both the Iterable and the ISeq. There is also support in here for using a direct key/value iterator via IMapIterable as introduced in CLJ-1499.

Questions/notes:

  • Could potentially check for Map or Iterable instead of IPersistentMap in RT.keys()/vals(). Not sure how common it is to pass normal Java maps to keys/vals.
  • The direct Iterable support vanishes once you move off the head of the keys or vals seq. So (rest (keys map)) does not have Iterable support. This is not really possible unless you hold an Iterator and advance it along with the seq, but that seemed to introduce all sorts of possibilities for badness. Since maps are unordered, it seems weird to rely on any ordering or processing only parts of any map, so I suspect doing this would be quite rare.
  • This patch depends on CLJ-1499 for IMapIterable.

Performance: I tested perf using criterium to benchmark as follows:

(use 'criterium.core)
(def m (zipmap (range 1000) (range 1000)))
(bench (reduce + (keys m)))
(bench (reduce + (vals m)))
(bench expr) 1.6.0 1.7.0-alpha5 alpha5 + clj-1499 + patch
(reduce + (keys m)) 69 µs 73 µs 44 µs
(reduce + (vals m)) 75 µs 77 µs 50 µs

Tests:

  • I added some basic tests for subseq and rsubseq as those both rely on the somewhat special behavior of keys accepting a seqable of Map.Entry objects (not just a map itself). There were no other tests for subseq or rsubseq already present.

Patch: clj-1602-4.diff - requires CLJ-1499 patch first (for IMapIterable)

Screened by:

  • the changes in CollReduce are unlikely to stand but are currently essential


 Comments   
Comment by Alex Miller [ 25/Nov/14 11:53 AM ]

Could leverage CLJ-1499 for the bulk of this, may pull that back from 1.8 into 1.7. Waiting on further work till that's answered.

Comment by Alex Miller [ 03/Dec/14 11:24 PM ]

I also have a patch that extends the CLJ-1499 iterators to support providing both key and val iterators that do not require creating and unpacking a Map.Entry. Unfortunately I only saw times that were ~48 µs on the perf benchmark in the description, so it's not a huge benefit (short-lived object allocation is cheap).

Comment by Alex Miller [ 09/Jan/15 9:15 AM ]

waiting on 1499 mods

Comment by Alex Miller [ 14/Jan/15 5:42 PM ]

Updated patch based on latest CLJ-1499-v10 patch.

Comment by Michael Blume [ 15/Jan/15 1:48 AM ]

I find it concerning that the v9 patch passed generative tests, shouldn't we have seen that?

Comment by Alex Miller [ 15/Jan/15 7:21 AM ]

Actually I did see it in the generative tests for this ticket so I moved those tests into the CLJ-1499 patch.

Comment by Michael Blume [ 15/Jan/15 10:10 AM ]

Ah, cool =)





[CLJ-1601] transducer arities for map-indexed, distinct, and interpose Created: 25/Nov/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: transducers

Attachments: Text File clj-1601-2.patch     Text File clj-1601-3.patch     Text File clj-1601-4.patch     Text File clj-1601.patch     Text File clj-1601-transient-distinct.patch    
Patch: Code and Test
Approval: Ok

 Description   
  • with generative tests
  • with examples demonstrating performance

Performance: Details in comments, summary:

(def v (vec (concat (range 1000) (range 1000))))
(into [] (distinct v))            ;; 821.3 µs
(into [] (distinct) v)            ;; 388.2 µs
(into [] (interpose nil v))       ;; 316.0 µs
(into [] (interpose nil) v)       ;; 35.5 µs
(into [] (map-indexed vector v))  ;; 76.8 µs
(into [] (map-indexed vector) v)  ;; 49.4 µs

Patch: clj-1601-4.patch

Screening note: We could use transients to improve performance of the distinct impl, except checking containment in a transient set is broken per CLJ-700 (which is not currently in 1.7). I have a new patch and direction on CLJ-700 that could provide a way to solve that if we want to move it back and push this further. Or we could just wait and refactor when CLJ-700 does go in.

Screened by:



 Comments   
Comment by Alex Miller [ 25/Nov/14 11:54 AM ]

working on this

Comment by Alex Miller [ 25/Nov/14 4:22 PM ]

Initial patch with impls. Tests and perf still to do.

Comment by Alex Miller [ 27/Nov/14 7:09 AM ]

Perf tests, summarized in description:

user=> (use 'criterium.core)
nil
user=> (def v (vec (concat (range 1000) (range 1000))))
#'user/v
user=> (quick-bench (into [] (distinct v)))
WARNING: Final GC required 10.433088780213309 % of runtime
Evaluation count : 744 in 6 samples of 124 calls.
             Execution time mean : 821.339608 µs
    Execution time std-deviation : 11.351053 µs
   Execution time lower quantile : 811.901435 µs ( 2.5%)
   Execution time upper quantile : 837.972000 µs (97.5%)
                   Overhead used : 1.794010 ns
nil
user=> (quick-bench (into [] (distinct) v))
WARNING: Final GC required 10.78492057474076 % of runtime
Evaluation count : 14028 in 6 samples of 2338 calls.
             Execution time mean : 43.630656 µs
    Execution time std-deviation : 170.185825 ns
   Execution time lower quantile : 43.433193 µs ( 2.5%)
   Execution time upper quantile : 43.853959 µs (97.5%)
                   Overhead used : 1.794010 ns
				   
user=> (quick-bench (into [] (interpose nil v)))
WARNING: Final GC required 10.79555726490133 % of runtime
Evaluation count : 1914 in 6 samples of 319 calls.
             Execution time mean : 316.024853 µs
    Execution time std-deviation : 9.077484 µs
   Execution time lower quantile : 310.139273 µs ( 2.5%)
   Execution time upper quantile : 330.917486 µs (97.5%)
                   Overhead used : 1.794010 ns

Found 1 outliers in 6 samples (16.6667 %)
	low-severe	 1 (16.6667 %)
 Variance from outliers : 13.8889 % Variance is moderately inflated by outliers
nil
user=> (quick-bench (into [] (interpose nil) v))
WARNING: Final GC required 10.70401297525592 % of runtime
Evaluation count : 17022 in 6 samples of 2837 calls.
             Execution time mean : 35.592672 µs
    Execution time std-deviation : 560.066138 ns
   Execution time lower quantile : 35.252348 µs ( 2.5%)
   Execution time upper quantile : 36.553414 µs (97.5%)
                   Overhead used : 1.794010 ns

Found 1 outliers in 6 samples (16.6667 %)
	low-severe	 1 (16.6667 %)
 Variance from outliers : 13.8889 % Variance is moderately inflated by outliers
nil

user=> (quick-bench (into [] (map-indexed vector v)))
WARNING: Final GC required 12.45755646853723 % of runtime
Evaluation count : 7338 in 6 samples of 1223 calls.
             Execution time mean : 76.807691 µs
    Execution time std-deviation : 381.019170 ns
   Execution time lower quantile : 76.433202 µs ( 2.5%)
   Execution time upper quantile : 77.170733 µs (97.5%)
                   Overhead used : 1.794010 ns
nil
user=> (quick-bench (into [] (map-indexed vector) v))
WARNING: Final GC required 11.38700971837483 % of runtime
Evaluation count : 12474 in 6 samples of 2079 calls.
             Execution time mean : 49.458043 µs
    Execution time std-deviation : 620.716737 ns
   Execution time lower quantile : 48.995801 µs ( 2.5%)
   Execution time upper quantile : 50.229507 µs (97.5%)
                   Overhead used : 1.794010 ns
Comment by Alex Miller [ 17/Dec/14 1:50 PM ]

Updated based on comment from Christophe Grand that java.util.HashSet used in distinct impl had different hash/equality semantics than the set used with sequences.

Comment by Nikita Prokopov [ 21/Dec/14 6:13 AM ]

This can be further improved by using transient set instead of persistent one in distinct:

distinct with persistent set, w/o transducers:  904.932406 µs
distinct with transient set,  w/o transducers:  755.338598 µs
distinct with persistent set, with transducers: 452.170600 µs
distinct with transient set,  with transducers: 293.258473 µs

Only caveat is that transient sets do not support contains? for now (see CLJ-700). This can be solved by using (.contains ^clojure.lang.ITransientSet set key)

I’m not sure what’s the best way to attach patch to this, for now attaching a patch that can be applied on top of Alex changes (clj-1601-transient-distinct.patch).

Comment by Alex Miller [ 22/Dec/14 8:32 AM ]

Hey Nikita, I'd rather fix CLJ-700 and use the normal functions rather than what you've done in the patch, which is why I hadn't done this before. I'm waiting to check with Rich whether we'll do that in 1.7 or wait till next release.

Comment by Michael Blume [ 09/Jan/15 10:23 AM ]

1601-3 no longer applies cleanly to master, I've got a reroll that does, is it ok to attach it even though the ticket is marked 'screened'?

Comment by Alex Miller [ 09/Jan/15 10:27 AM ]

Updated tests to apply cleanly to current master in -4 patch.

Comment by Alex Miller [ 09/Jan/15 10:33 AM ]

Ha, didn't see your comment Michael! I was working on the same thing.





[CLJ-1600] calling hashCode on clojure.lang.LazyTransformer causes a StackOverflowError Created: 24/Nov/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Defect Priority: Major
Reporter: Sam Ritchie Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: transducers
Environment:

OS X 10.10.1, Macbook Pro,, Java 1.8.0_11, Clojure 1.7.0-alpha4


Attachments: Text File CLJ-1600-2.patch     Text File CLJ-1600.patch    
Patch: Code and Test
Approval: Ok

 Description   

Calling .hashCode on a an instance of clojure.lang.LazyTransformer causes a StackOverflowError:

user> (.hashCode (sequence (map identity) ["s"]))
StackOverflowError   clojure.lang.Util.hash (Util.java:161)

The trace is

Util.java:  161  clojure.lang.Util/hash
          LazyTransformer.java:  216  clojure.lang.LazyTransformer/hashCode
                     Util.java:  161  clojure.lang.Util/hash
          LazyTransformer.java:  216  clojure.lang.LazyTransformer/hashCode
                     Util.java:  161  clojure.lang.Util/hash
          LazyTransformer.java:  216  clojure.lang.LazyTransformer/hashCode
                     Util.java:  161  clojure.lang.Util/hash
          LazyTransformer.java:  216  clojure.lang.LazyTransformer/hashCode

Relevant lines:

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LazyTransformer.java#L212
https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Util.java#L161

Cause: Looks like "seq" returns "this":

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LazyTransformer.java#L55

This does NOT occur on an empty sequence, as clojure.core/sequence short-circuits.

Proposal: compute and cache hash and hasheq using same algorithm used in other seqs

Patch: CLJ-1600-2.patch

Screened by: Alex Miller



 Comments   
Comment by Ghadi Shayban [ 25/Nov/14 1:18 AM ]

Patch with hashcode calculation and caching similar to ASeq. Might be worthwhile hoisting that into its own hashSeq method.

Comment by Alex Miller [ 25/Nov/14 10:13 AM ]

What's here looks good. Can we hook into existing tests that verify equals/hashcode and equiv/hasheq equivalence?

Comment by Ghadi Shayban [ 25/Nov/14 1:24 PM ]

Test case added. Verified case was failing with SO prior to patch.





[CLJ-1599] Add a reset! that returns old value Created: 24/Nov/14  Updated: 02/Mar/15

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

Type: Enhancement Priority: Major
Reporter: Steven Yi Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: atom

Attachments: File get-and-set.diff    
Patch: Code
Approval: Triaged

 Description   

DESCRIPTION

This patch adds the equivalent of reset!, here called get-and-set!, to core to allow getting the last value from an atom and setting it to a new value. This is useful for atomically draining an atom of its value and setting to a new value. The implementation delegates to Java's AtomicReference.getAndSet().

The equivalent operation in Clojure code would be:

(defn get-and-set! [atm newv]
(loop [oldv @atm]
(if (compare-and-set! atm oldv newv)
oldv
(recur @atm))))

This is close to a 1:1 translation of the Java code in sun.misc.Unsafe's getAndSetObject, used by AtomicReference (as of current JDK9 source code).

APPLICATIONS

  • User may want to check if an operation has occurred before by using an atom as a flag. I.e.,

(def has-run-once (atom false))
...
(when-not (get-and-set! has-run-once true)
(do something))

  • User may want to use an atom similarly to a java.util.concurrent.LinkedTransferQueue, for the case of pairing up adds by writers and drainTo by readers:

Thread 1: (swap! atm conj item1)
Thread 2: (swap! atm conj item2)
Thread 3: (let [new-vals (get-and-set! atm [])]
(do-something new-vals))

ALTERNATIVES

  • For case of atom as flag, user can use existing compare-and-set!:

(def has-run-once (atom false))
...
(when-not (compare-and-set! has-run-once false true)
(do something))

Argument: get-and-set! is a little clearer in intent as it is using the value of the atom, rather than the success of the cas operation. Also, this would not be applicable to situations where the value stored is not a boolean.

  • User can just go ahead and use LinkedTransferQueue.

Argument: User not fluent in Java may not be readily able to use this.

==

Argument for: This seems like a sufficiently primitive operation to include in core for atoms. I am unsure of the rationale, but assume it was vetted to include into Java's AtomicReference for a reason. Also, if users are using atoms and have this available, they are less likely to try to do this incorrectly, such as:

(let [vals @some-atom]
(reset! some-atom [])
(do-something-with vals))

Argument against: This may not be sufficiently primitive enough to include in core. Users have a workaround to implement the get-and-set! operation in user-code as given above.

Note: This request is similar to CLJ-1454 (http://dev.clojure.org/jira/browse/CLJ-1454), but differs in that this is not a swap! operation that accepts an IFn argument. Also, I looked to add a test in test/clojure/test_clojure/atoms.clj, but saw that the other operations weren't tested. (I assume this is due to the other operations delegating to AtomicReference and hence not deemed test-worthy.)



 Comments   
Comment by Michael Blume [ 02/Mar/15 5:03 PM ]

I tend to wind up inevitably adding this to my projects, be nice to have it in core

Comment by Alex Miller [ 02/Mar/15 7:27 PM ]

So CLJ-1454 is swap! and return old and CLJ-1599 is reset! and return old?

Comment by Steven Yi [ 02/Mar/15 7:48 PM ]

Yes, I think that's an accurate interpretation of the two tickets.





[CLJ-1598] Make if forms compile directly to the appropriate branch expression if the test is a literal Created: 24/Nov/14  Updated: 24/Nov/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: 1
Labels: compiler, performance, primitives

Attachments: Text File 0001-if-test-expr-of-an-if-statement-is-a-literal-don-t-e.patch    
Approval: Triaged

 Description   

This allows expressions like `(cond (some-expr) 1 :else 2)` to be eligible for unboxed use, which is not currently possible since the cond macro always ends up with a nil else branch that the compiler currently takes into account.

With the attached patch, the trailing (if :else 2 nil) in the macroexpansion will be treated as 2 by the Compiler, thus allowing the unboxed usage of the cond expression.






[CLJ-1597] Allow ISeq args to map conj Created: 22/Nov/14  Updated: 22/Nov/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: collections, maps

Attachments: Text File 0001-allow-ISeq-args-to-map-conj.patch    
Patch: Code and Test

 Description   

Currently conj on maps can take only maps or vectors, this patch allows it to take any ISeq:

user=> (conj {} '(1 2))
{1 2}





[CLJ-1596] Using keywords in place of symbols for defrecord fields causes a compiler exception with incorrect line number Created: 20/Nov/14  Updated: 20/Nov/14

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

Type: Defect Priority: Minor
Reporter: Kyle Kingsbury Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, defrecord

Approval: Triaged

 Description   

Possibly related to http://dev.clojure.org/jira/browse/CLJ-1261: a defrecord like

(defn foo [x])

(defrecord Bar [:b])

Throws an exception, like you'd expect:

java.lang.ClassCastException: clojure.lang.Keyword cannot be cast to clojure.lang.IObj, compiling:(tesser/quantiles_test.clj:45:15)

However, this exception's line and character indicates the error is in the previous form: the defn, not the defrecord. This can be really tricky to figure out when the expressions are more complicated.



 Comments   
Comment by Alex Miller [ 20/Nov/14 4:17 PM ]

Related: CLJ-1261

Comment by Alex Miller [ 20/Nov/14 4:18 PM ]

Possibly fixed by CLJ-1561, not sure.





[CLJ-1595] Nested doseqs leak with sequence of huge lazy-seqs Created: 20/Nov/14  Updated: 20/Nov/14

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

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

Attachments: File doseq_leaks.diff    
Patch: Code

 Description   

Hello!

This little snippet demonstrates the problem:

(doseq [outer-seq (list (range)) inner-seq outer-seq])

That's it. It is not just eats my processor, but also eats all available memory. Practically it can affect (and it is) at consuming of complex lazy structures like huge XML-documents.

I think this is at least non trivial behaviour.

It can be fixed by this small patch. We can get next element before current iteration, not after, so outer loop will not hold reference to the head of inner-seq.

This patch doesn't solve all problems, for example this code:

(doseq [outer-seq [(range)] inner-seq outer-seq])

leaks. Because chunked-seqs (vector in this case) holds current element by design.






[CLJ-1594] Colons followed by spaces are not ignored within (comment ...) Created: 19/Nov/14  Updated: 22/Nov/14  Resolved: 22/Nov/14

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

Type: Defect Priority: Major
Reporter: Ed Ward Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug, comment, function
Environment:

Ubuntu Linux 14.10
Clojure 1.6.0
OpenJDK 64-Bit Server VM 1.7.0_65-b32



 Description   

Running

(comment abc:def)
works, while running
(comment abc: def)
and
(comment abc : def)
fail with the exception

RuntimeException Invalid token: :  clojure.lang.Util.runtimeException (Util.java:221)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: def in this context, compiling:(/tmp/form-init1585368677683647130.clj:1:1010) 
RuntimeException Unmatched delimiter: )  clojure.lang.Util.runtimeException (Util.java:221)


 Comments   
Comment by Nicola Mometto [ 19/Nov/14 11:19 AM ]

`comment` is a macro, it doesn't bypess the reader.
":" is not a regular clojure token so it's expected that this throws.
The only way to include non clojure tokens in a source code is after a ";" comment

Comment by Andy Fingerhut [ 19/Nov/14 3:37 PM ]

I've just added some notes about this to ClojureDocs.org here: http://clojuredocs.org/clojure.core/comment

Comment by Alex Miller [ 22/Nov/14 9:12 AM ]

agreed with Nicola's comment above





[CLJ-1593] Use PAM for small maps when assigned to a var rather than always using PHMs Created: 15/Nov/14  Updated: 08/Dec/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: collections, compiler, maps

Attachments: Text File 0001-Use-PAM-rather-than-always-using-PHMs-for-small-maps.patch    
Patch: Code

 Description   

I'm reproposing the fix I implemented for http://dev.clojure.org/jira/browse/CLJ-944 a while ago as an enhancement rather than as a defect.

Currently when a map is used as the value of a `def` expression, unless it's an empty map, it will always be a PersistentHashMap even if it's a small map.

user=> (def a {:foo :bar})
#'user/a
user=> (class a)
clojure.lang.PersistentHashMap

The current patch makes makes small maps be compiled to PAMs, consistently with how it's handled in lexical contexts, only using PHMs when the number of elements is above the threshold

user=> (def a {:foo :bar})
#'user/a
user=> (class a)
clojure.lang.PersistentArrayMap
user=> (class (let [a {:foo :bar}] a))
clojure.lang.PersistentArrayMap
user=> (def a {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7 8 8 9 9})
#'user/a
user=> (class a)
clojure.lang.PersistentHashMap


 Comments   
Comment by Alex Miller [ 15/Nov/14 12:17 PM ]

This might be subsumed under the small collections CLJ-1517, not sure.

Comment by Nicola Mometto [ 08/Dec/14 9:19 AM ]

This is now out of scope for CLJ-1517 now that's focused only on vectors.

Comment by Alex Miller [ 08/Dec/14 9:47 AM ]

We're just splitting the ticket apart, maps will be a separate ticket/patch.





[CLJ-1592] Ability to suppress warnings on name conflict with clojure.core Created: 14/Nov/14  Updated: 14/Nov/14

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

Type: Enhancement Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

In numerical code, it is often useful and idiomatic to replace clojure.core functions with augmented versions (e.g. clojure.core.matrix.operators defines + in a way that works with whole arrays, not just scalar numbers)

Currently there seems to be no way to avoid a warning in client code when a library does this, e.g.:

;; library namespace
(ns foo
  (:refer-clojure :exclude [+]))
(def + clojure.core/+)

;; later on, in some other namespace
(require '[foo :refer :all])
=> WARNING: + already refers to: #'clojure.core/+ in namespace: bar, being replaced by: #'foo/+

A workaround exists by using (:refer-clojure :exclude ...) in the user namespace, however this creates unnecessary work for the user and requires maintenance of boilerplate code.

Proposed solution is to allow vars to be annotated with additional metadata (e.g. ^:replace-var ) that when added to library functions will suppress this warning. This will allow library authors to specify that a function should work as a drop-in replacement for clojure.core (or some other namespace), and that a warning is therefore not required.



 Comments   
Comment by Andy Fingerhut [ 14/Nov/14 9:46 PM ]

Duplicate with CLJ-1257 ?

Comment by Mike Anderson [ 14/Nov/14 9:53 PM ]

Hi Andy, it refers to the same warning - but the scope of the solution is different:

  • CLJ-1257 is more like a global way to turn off this warning
  • CLJ-1592 is for suppressing this warning on specific vars

If CLJ-1257 is implemented and the warning is off be default, CLJ-1592 becomes mostly unnecessary. Without CLJ-1257 or if the warning defaults to on, CLJ-1592 is needed.





[CLJ-1591] Symbol not being bound in namespace when name clashes with clojure.core Created: 14/Nov/14  Updated: 28/Dec/14

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

Type: Defect Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Approval: Triaged

 Description   

The following code fails (both in 1.6 and latest 1.7-alpha4):

user=> (ns foo)
nil
foo=>  (def inc inc)
WARNING: inc already refers to: #'clojure.core/inc in namespace: foo, being replaced by: #'foo/inc
#'foo/inc

;; Note inc is unbound at this point, which causes the exception below
foo=> inc
#<Unbound Unbound: #'foo/inc>
foo=> (ns bar)
nil
bar=> (require ['foo :refer ['inc]])
WARNING: inc already refers to: #'clojure.core/inc in namespace: bar, being replaced by: #'foo/inc
nil
bar=> (inc 8)

IllegalStateException Attempting to call unbound fn: #'foo/inc  clojure.lang.Var$Unbound.throwArity (Var.java:43)

Further investigation shows that foo/inc is unbound:

foo/inc
=> #<Unbound Unbound: #'foo/inc>

Further investigation also shows that replacing the (def inc inc) with almost anything else, e.g. (def inc dec), (def inc clojure.core/inc), or (def inc (fn [n] (+ n 1))), causes no exception (but the warnings remain).

I would expect:
a) foo/inc should be bound and have the same value as clojure.core/inc
b) No error when requiring foo/inc
c) bar/inc should be bound to foo/inc



 Comments   
Comment by Nicola Mometto [ 14/Nov/14 10:04 PM ]

The second error should be expected, the right syntax should be (require ['foo :refer ['inc]]) (note the leading quote before inc)

Comment by Mike Anderson [ 14/Nov/14 10:20 PM ]

Thanks for the catch Nicola - I've edited the description. Still get the same error however (just with a slightly different message)

Comment by Alex Miller [ 14/Nov/14 10:22 PM ]

See comment...

Comment by Mike Anderson [ 14/Nov/14 10:24 PM ]

@Alex what comment? Note that the error still occurs even with the right syntax....

Comment by Mike Anderson [ 14/Nov/14 10:26 PM ]

Appears to have been closed prematurely

Comment by Alex Miller [ 14/Nov/14 10:39 PM ]

I can't reproduce with the correct syntax:

Clojure 1.7.0-master-SNAPSHOT
user=> (ns foo)
nil
foo=> (def inc inc)
WARNING: inc already refers to: #'clojure.core/inc in namespace: foo, being replaced by: #'foo/inc
#'foo/inc
foo=> (ns bar)
nil
bar=> (require ['foo :refer ['inc]])
WARNING: inc already refers to: #'clojure.core/inc in namespace: bar, being replaced by: #'foo/inc
nil
Comment by Mike Anderson [ 14/Nov/14 10:55 PM ]

The problem is that the var is still unbound and causes e.g. the following error:

=> (foo/inc 8)
IllegalStateException Attempting to call unbound fn: #'foo/inc clojure.lang.Var$Unbound.throwArity (Var.java:43)

I don't think that should be expected - or am I missing something?

Comment by Alex Miller [ 14/Nov/14 10:57 PM ]

Ah, will take a look. But not right now.

Comment by Andy Fingerhut [ 15/Nov/14 1:09 PM ]

Updated the description with a few more details. The exception goes away if you do (def inc (fn [n] (+ n 1))) instead of (def inc inc), for example. The warnings remain.

Comment by Tom Crayford [ 20/Nov/14 11:07 AM ]

Unsure if this is the same issue (I think it might be?), but I reproduced the exact same error message with AOT compilation involved:

reproduced in this git repository: https://github.com/yeller/compiler_update_not_referenced_bug

clone it, run `lein do clean, uberjar, test`, and that error message will show up every time for me

Comment by Andy Fingerhut [ 20/Nov/14 5:43 PM ]

Mike, I think replacing (def inc inc) in your example with (def inc clojure.core/inc) should be considered as a reasonable workaround for this issue, unless you have some use case where you need to def inc to something that is not in clojure.core (and if so, why?)

The reason (def inc inc) behaves this way is, if not absolutely necessary, at least commonly used in Clojure programs to define recursive functions, e.g. (defn fib [n] (if (<= n 1) 1 (+ (fib (dec n)) (fib (- n 2))))), so that the occurrences of fib in the body are resolved to the fib being defined.

Comment by Alex Miller [ 22/Nov/14 9:05 AM ]

Moving to 1.7 until I can look at this more deeply.

Comment by Mike Anderson [ 23/Nov/14 6:08 PM ]

Andy - yes the workaround is fine for me right now.

I don't think this is an urgent issue but it may be exposing a subtle complexity regarding assumptions about the state of the namespace at different times. Perhaps the semantics should be something like:

  • The def statement itself should be run before the var is interned. e.g. (def inc (inc 5)) should result in (def inc 6)
  • Anything complied / deferred to run after completion of the def statement should use the new var (i.e. the new var should be referenced by fns, lazy sequences etc.)
Comment by Andy Fingerhut [ 23/Nov/14 6:36 PM ]

I'm not sure what your proposal means in a case like this:

(def inc (fn [x] (inc x)))

Is the second inc to be interpreted/resolved before or after the new inc is created? Because it is (fn ...) it should be the after-behavior? What else besides fn should cause the after-behavior, rather than the before-behavior?

Even more fun (not saying that people often write code like this, but the compiler can handle it today):

(def inc (if (> (inc y) 5)
           (fn [x] (inc x))
           (fn [x] (dec x))))

I think the current compiler behavior of 'in the body of a def, the def'd symbol always refers to the new var, not any earlier def'd vars' is fairly straightforward to explain.

Comment by Tom Crayford [ 23/Nov/14 9:15 PM ]

Should I file the AOT issue reproduced in that thing as a new issue?

Comment by Andy Fingerhut [ 24/Nov/14 5:16 PM ]

Tom: Alex Miller or another screener would be best to say whether the AOT issue should be a separate ticket, but my best guess would be "go for it". I tried to look at the link you gave but it seems not to point to anything. Could you double-check that link?

Comment by Tom Crayford [ 24/Nov/14 6:48 PM ]

Andy,

Great. I'll write one up tomorrow sometime. I accidentally left that repo as private, should be visible now.

Comment by Andy Fingerhut [ 24/Nov/14 8:11 PM ]

This comment is really most relevant for ticket CLJ-1604, where it has been copied:

Tom, looked at your project. Thanks for that. It appears not to have anything like (def inc inc) in it. It throws exception during test step of 'lein do clean, uberjar, test' consistently for me, too, but compiles with only warnings and passes tests with 'lein do clean, test'. I have more test results showing in which Clojure versions these results change. To summarize, the changes to Clojure that appear to make the biggest difference in the results are below (these should be added to the new ticket you create – you are welcome to do so):

Clojure 1.6.0, 1.7.0-alpha1, and later changes up through the commit with description "CLJ-1378: Allows FnExpr to override its reported class with a type hint": No errors or warnings for either lein command above.

Next commit with description "Add clojure.core/update, like update-in but takes a single key" that adds clojure.core/update: 'lein do clean, test' is fine, but 'lein do clean, uberjar' throws exception during compilation, probably due to CLJ-1241.

Next commit with description "fix CLJ-1241": 'lein do clean, test' and 'lein do clean, uberjar' give warnings about clojure.core/update, but no errors or exceptions. 'lein do clean, uberjar, test' throws exception during test step that is same as the one I see with Clojure 1.7.0-alpha4. Debug prints of values of clojure.core/update and int-map/update (in data.int-map and in Tom's namespace compiler-update-not-referenced-bug.core) show things look fine when printed inside data.int-map, and in Tom's namespace when not doing the uberjar, but when doing the uberjar, test, int-map/update is unbound in Tom's namespace.

In case it makes a difference, my testing was done with Mac OS X 10.9.5, Leiningen 2.5.0 on Java 1.7.0_45 Java HotSpot(TM) 64-Bit Server VM

Comment by Nicola Mometto [ 25/Nov/14 3:44 PM ]

Tom, I've opened a ticket with a patch fixing the AOT issue: http://dev.clojure.org/jira/browse/CLJ-1604





[CLJ-1590] Some IReduce/IReduceInit implementors don't respect reduced Created: 14/Nov/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

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

Attachments: Text File 0001-ensure-IReduce-IReduceInit-implementors-respect-redu.patch     Text File clj-1537-gvec-ArraySeq.patch    
Patch: Code and Test
Approval: Ok

 Description   

Several reduce implementations don't properly respect reduced:

  • clojure.core.ArrayChunk's implementation of IChunk/reduce
  • VecSeq's impl of InternalReduce/reduce
  • APersistentVector's reduce with init doesn't unwrap reduced on last value
  • seqs of primitive arrays don't unwrap reduced on last value
  • PersistentList doesn't unwrap reduced on last value

Some examples:

user=> (transduce (take 1) conj (seq (long-array [1 2 3 4])))
#<Reduced@38f774f8: [1]>
user=> (.reduce (list 1 2 3 4 5) (fn [_ a] (if (= a 5) (reduced "foo"))) 1)
#<Reduced@753d01cc: "foo">

Patch: 0001-ensure-IReduce-IReduceInit-implementors-respect-redu.patch
Screened by: Alex Miller
See also: http://dev.clojure.org/jira/browse/CLJ-1537



 Comments   
Comment by Alex Miller [ 17/Nov/14 12:35 PM ]

The patch should only be considering the result of calling the reducing function, not checking the init value (this matches what we do elsewhere).

Also, needs at least some simple example tests.

Comment by Nicola Mometto [ 17/Nov/14 1:36 PM ]

While reworking my patch to address your comment, I discovered that PersistentList and APersistentList's IReduceInit/reduce implementation aren't handling correctly reduced when the reducing function returns one on the last iteration.

The attached patch fixes those too and contains testcases demonstrating the issues.

Comment by Nicola Mometto [ 17/Nov/14 1:39 PM ]

I haven't fixed the IReduce/IReduceInit implementations for range as that's in scope for http://dev.clojure.org/jira/browse/CLJ-1515

Comment by Nicola Mometto [ 17/Nov/14 1:59 PM ]

As Ghadi Shayban noticed, while reduce doesn't use IReduceInit's reduce impl for PersistentList, transduce does so this might be cause of serious bugs even from clojure code, not only when using `.reduce` calls

Comment by Alex Miller [ 17/Nov/14 2:47 PM ]

reduce will use IReduceInit's reduce impl for PersistentList, after CLJ-1572.





[CLJ-1589] Cleanup internal-reduce implementation Created: 14/Nov/14  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-cleanup-internal-reduce-impl.patch    
Patch: Code
Approval: Ok

 Description   

Currently internal-reduce provides an implementation for ArraySeq and the ArraySeq_* prim classes.
Since those classes implement IReduce the current patch makes instances of those classes fallback on coll-reduce's IReduce impl (that simply invokes .reduce)

This change is desiderable because it removes unnecessary duplicated code, reducing the implementation surface and making it easier to follow reduce's code path. In addition to ArraySeq there will be (based on other tickets) more seq impls that also IReduce, so it would be good to re-route back through coll-reduce when we get combinations of potentially reducible sub-seqs.

Patch: 0001-cleanup-internal-reduce-impl.patch

  • This patch depends on the patch for CLJ-1590 since the current IReduce impl for those ArraySeq classes doesn't properly handle Reduced

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 14/Nov/14 10:28 PM ]

I'm not sure whether this should be in 1.7 or not, but I'm adding it there so we can have a discussion on it regardless.





[CLJ-1588] StackOverflow in clojure.test macroexpand with `are` and anonymous `fn` Created: 13/Nov/14  Updated: 28/Dec/14

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

Type: Defect Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File clojure-test.recursion.patch    
Approval: Triaged

 Description   

I noticed that this happens when an argument in anonymous `fn` is named the same as one of the binding in `are` form

(use 'clojure.test)
(deftest x
  (are [x y] (= x y)
    ((fn [x] (inc x)) 1) 2))
=>
clojure.lang.Compiler$CompilerException: java.lang.StackOverflowError, compiling:(/Users/nprokopov/Dropbox/ws/clojure.unicode/test/clojure/test_unicode.clj:54:3)

This path contains fix & test:

clojure-test.recursion.patch

src/clj/clojure/template.clj => line 43
test/clojure/test_clojure/test.clj => lines 83-85



 Comments   
Comment by Michael Blume [ 13/Nov/14 11:45 PM ]

yep, I bet it's trying to replace the x with (fn [x] (inc x)) and then replacing the x in that and...

this doesn't seem like that much of a defect? Like why write the code with the same variable names in the first place?

Comment by Nikita Prokopov [ 13/Nov/14 11:49 PM ]

Well, logically these are two completely separate, isolated Xes. It can be avoided, sure, but this behavior is not expected and I’m sure can be easily fixed.

Comment by Nikita Prokopov [ 13/Nov/14 11:53 PM ]

I mean, there shouldn’t be any recursion at all in the first place, right?

Comment by Nikita Prokopov [ 14/Nov/14 2:12 AM ]

Patch

Comment by Nikita Prokopov [ 14/Nov/14 2:13 AM ]

I fixed the issue by replacing prewalk with postwalk. It was caused by prewalk-replace that first replaced the form and then goes inside it looking for more replacements. Postwalk-replace avoids that.

Comment by Alex Miller [ 14/Nov/14 9:27 AM ]

1) Needs tests.
2) As a change in template, this affects many possible users. Can you assess other possible users either in core or in other external projects and whether they are affected? I have found http://crossclj.info to be helpful for questions like this.

Comment by Alex Miller [ 14/Nov/14 11:42 AM ]

Please include tests in a single combined patch and update the description to include a line specifying the current active patch for consideration.

Comment by Nikita Prokopov [ 14/Nov/14 11:46 AM ]

Alex, I attached a test case.

I also took a look at all use-cases of apply-template and do-template in both clojure.core and third-party projects. It’s not used very often, and in all cases use of do-template is pretty straightforward (take [x y z] and just replace it with some completely different forms), it does not depend on recursion and change from prewalk to postwalk will not cause any change in behavior. I think this patch is safe.

Comment by Nikita Prokopov [ 14/Nov/14 12:00 PM ]

I’m afraid I cannot edit description...

src/clj/clojure/template.clj => line 43
test/clojure/test_clojure/test.clj => lines 83-85

Comment by Alex Miller [ 14/Nov/14 10:14 PM ]

You should have edit rights now on jira issues.





[CLJ-1587] PersistentArrayMap's assoc doesn't respect HASHTABLE_THRESHOLD Created: 12/Nov/14  Updated: 12/Nov/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections, data-structures, maps

Attachments: Text File 0001-PersistentArrayMap-s-assoc-doesn-t-respect-HASHTABLE.patch    
Patch: Code

 Description   

Currently a map with more than 8 elements will be converted from a PersistentArrayMap to a PersistentHashMap, but if using assoc, it will take 9 elements before the conversion happens:

user=>  (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7})
clojure.lang.PersistentArrayMap
user=> (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7 8 8})
clojure.lang.PersistentHashMap
user=>  (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8))
clojure.lang.PersistentArrayMap
user=>  (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8 9 9))
clojure.lang.PersistentHashMap

After patch:

user=> (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7})
clojure.lang.PersistentArrayMap
user=> (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7 8 8})
clojure.lang.PersistentHashMap
user=> (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8))
clojure.lang.PersistentHashMap
user=> (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8 9 9))
clojure.lang.PersistentHashMap





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

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

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

Attachments: Text File 0001-Compiler-doesn-t-preserve-metadata-for-lazyseq-liter.patch    
Patch: Code
Approval: Triaged

 Description   

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

Example demonstrating this issue:

user=> (set! *warn-on-reflection* true)
true
user=> (list '.hashCode (with-meta (concat '(identity) '("foo")) {:tag 'String}))
(.hashCode (identity "foo"))
user=> (eval (list '.hashCode (with-meta (concat '(identity) '("foo")) {:tag 'String})))
Reflection warning, NO_SOURCE_PATH:1:1 - reference to field hashCode can't be resolved.
101574

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

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

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

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

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






[CLJ-1585] Report boxed math warning on function that boxes primitive return value Created: 11/Nov/14  Updated: 28/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, math

Attachments: Text File clj-1585.patch    
Patch: Code
Approval: Triaged

 Description   

With the new :warn-on-boxed (CLJ-1325), these examples do not report a boxed math warning although they each do boxing:

user=> (defn f1 [^long x] (inc x))
f1
user=> (defn f2 [x] (aget (long-array [1 2]) 0))
f2
user=> (defn f3 [x] (aget (int-array [1 2]) 0))
f3
user=> (defn f4 [^String s] (.indexOf s "a"))

Cause: emitBoxReturn has a hard-coded call to box a prim return value.

Solution: If *unchecked-math* is set to :warn-on-boxed, emit a warning on boxing of primitive numeric return types.

Patch:



 Comments   
Comment by Alex Miller [ 12/Nov/14 12:39 AM ]

Attached patch does the job, but from trying it out on some real code, it finds both problematic cases and lots of cases that could safely be ignored and/or where there is no obvious way to fix the warning. I think it may need some more tuning to reduce the rate of unfixable things a bit.





[CLJ-1584] unfair atom update Created: 09/Nov/14  Updated: 09/Nov/14  Resolved: 09/Nov/14

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

Type: Defect Priority: Minor
Reporter: Nikolay Ryzhikov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

Is it by design?

atom use for(; and compareAndSet to update value and does not care temporal order of updates

If one repetitive thread more active then other,
then slower never get a chance to update, until faster stop.

Example: https://gist.github.com/niquola/f6ec8ddfaa2a56ea6257



 Comments   
Comment by Alex Miller [ 09/Nov/14 7:57 PM ]

This is by design - it's rare in typical Clojure to be hammering an atom like this. If you really need fairness or fast counters, use JDK constructs like a fair ReentrantLock or the new adder classes.





[CLJ-1583] Apply forces the evaluation of one element more than necessary Created: 07/Nov/14  Updated: 09/Nov/14

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

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

Attachments: Text File 0001-make-RT.boundedLength-lazier.patch    
Patch: Code

 Description   

Given a function with one fixed argument and a vararg, it should be sufficient to force evaluation of 2 elements for apply to know which arity it should select, however it currently forces 3:

user=> (defn x ([a & b]))
#'user/x
user=> (apply x (map println (iterate inc 0)))
0
1
2
nil

This makes lazy functions that use apply (for example mapcat) less lazy than they could be.
The proposed patch makes RT.boundedLength short-circuit immediately after the seq count is greater than the max fixed arity:

user=> (defn x ([a & b]))
#'user/x
user=> (apply x (map println (iterate inc 0)))
0
1
nil


 Comments   
Comment by Nicola Mometto [ 09/Nov/14 3:37 PM ]

The patch in this ticket slightly improves the issue reported at http://dev.clojure.org/jira/browse/CLJ-1218





[CLJ-1582] Overriding in-ns and ns is problematic Created: 07/Nov/14  Updated: 07/Nov/14

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-Allow-overriding-of-clojure.core-in-ns-and-clojure.c.patch    
Patch: Code

 Description   

Currently it is possible to override clojure.core/in-ns and clojure.core/ns, but it is not possible to refer to the namespace-specific vars without fully qualifying them:

user=> (ns foo (:refer-clojure :exclude [in-ns]))
nil
foo=> (def in-ns 1)
#'foo/in-ns
foo=> in-ns
#<clojure.lang.RT$1@76b5e4c5>

After this patch, overriding in-ns and ns works like for every other clojure.core var:

user=> (ns foo (:refer-clojure :exclude [in-ns]))
nil
foo=> (def in-ns 1)
#'foo/in-ns
foo=> in-ns
1


 Comments   
Comment by Reid McKenzie [ 07/Nov/14 11:46 AM ]

This is motivated by https://github.com/jonase/eastwood/issues/100





[CLJ-1581] Inconsistent behavior in transient sets: they should allow contains? Created: 06/Nov/14  Updated: 06/Nov/14  Resolved: 06/Nov/14

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

Type: Defect Priority: Major
Reporter: Pierre-Yves Ritschard Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: bug, patch, transient

Attachments: Text File transient.patch    
Patch: Code and Test

 Description   

Transient maps and sets retain the behavior of persistent maps and sets.

When threading operations on transient sets, it is unfortunately impossible to test for membership, since the implementation of contains? defers to contains in clojure.lang.RT which does not

There are several solutions for this, I chose to extend contains in clojure.lang.RT to handle ITransientSet



 Comments   
Comment by Alex Miller [ 06/Nov/14 7:04 AM ]

Dupe of CLJ-700





[CLJ-1580] Transient collections should guarantee thread visibility Created: 05/Nov/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: transient

Attachments: Text File clj-1580-2.patch     Text File clj-1580.patch    
Patch: Code
Approval: Ok

 Description   

With changes from CLJ-1498, transients are still thread isolated but may move between threads during their lifetime which introduces new concurrency concerns, namely visibility of changes across threads.

Approach: Make all transient collection fields either final or volatile to ensure visibility across threads.

Patch: clj-1580-2.patch

Screened by:



 Comments   
Comment by Stuart Halloway [ 02/Jan/15 11:39 AM ]

Should ATtransientSet.impl be volatile also?

Comment by Alex Miller [ 02/Jan/15 11:57 AM ]

Added -2 patch that makes ATransientSet.impl volatile.





[CLJ-1579] source-fn can fail due to reading namespace-aliased keywords while in another namespace context Created: 05/Nov/14  Updated: 21/Nov/14

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

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

Attachments: Text File 0001-Read-src-in-appropriate-ns-context.patch    
Patch: Code
Approval: Triaged

 Description   

clojure.repl/source-fn functions by using a custom reader to read a source form at the location specified by line & file metadata on a given symbol. While this works well for most things, I encountered an issue when applying source-fn to code containing keyword namespace aliases ala ::T/foo. ::T/foo is a legitimate namespace keyword in the context where it occurs, because a namespace alias to T is created in the ns header. When the keyword ::T/foo is read then, it resolves to :my-other.ns/foo as one would expect because ns has the appropriate alias. However when attempting to read source via clojure.repl/source-fn, ns may no longer be the original read context of the indicated form thus leading to the erroneous exception java.lang.RuntimeException: Invalid token: ::T/foo.

The solution is that the reading operation of clojure.repl/source-fn must be wrapped in (binding [*ns* (.ns v)] ...) so that source reading will take place in the original load reading context thus preventing this error.

A patched equivalent function exists here, https://github.com/clojure-grimoire/lein-grim/blob/master/src/grimoire/doc.clj#L50-L74, and I will submit a patch against 1.6.0 in the morning.






[CLJ-1578] 1.7.0-alpha3 breakage due to symbol conflicts Created: 31/Oct/14  Updated: 14/Nov/14  Resolved: 14/Nov/14

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

Type: Defect Priority: Critical
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-1578-don-t-throw-when-a-core-Var-replaces-anothe.patch    
Patch: Code
Approval: Ok

 Description   

I've been trying to build core.matrix with 1.7.0-alpha3 and I get a failures due to symbol conflicts with clojure.core (specifically the new update function).

java.lang.IllegalStateException: update already refers to: #'clojure.core.matrix.utils/update in namespace: clojure.core.matrix.impl.ndarray-magic
	at clojure.lang.Namespace.warnOrFailOnReplace(Namespace.java:88)
	at clojure.lang.Namespace.reference(Namespace.java:110)
	at clojure.lang.Namespace.refer(Namespace.java:168)
	at clojure.core$refer.doInvoke(core.clj:4071)
	at clojure.lang.RestFn.invoke(RestFn.java:439)
	at clojure.core.matrix.impl.ndarray_magic$eval9762$loading__5295__auto____9763.invoke(ndarray_magic.clj:1)
	at clojure.core.matrix.impl.ndarray_magic$eval9762.invoke(ndarray_magic.clj:1)

Simpler case to reproduce:

(ns foo)
(def inc dec) ;; gets a warning 
(ns bar (:require [foo :refer :all])) ;; gets another warning
(ns bar (:require [foo :refer :all])) ;; causes the exception (effectively a ns reload)

Cause: In the case of a load, foo/inc is replacing clojure.core/inc and that causes the expected warning. In the case of a reload, clojure.core/inc is replacing foo/inc - this case is not currently handled and falls into the error case.

Approach: In the case of clojure.core/inc replacing foo/inc (should only happen during a reload), ignore and issue neither warning or error.

Patch: 0001-CLJ-1578-don-t-throw-when-a-core-Var-replaces-anothe.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 01/Nov/14 7:12 AM ]

The warnings I would expect / the failures I would not. Can you boil down the reproduction of the exception somehow?

Comment by Nicola Mometto [ 01/Nov/14 7:32 AM ]

I have seen similar failures when re-compiling a namespace that shadows a core Var:

  • ns foo is created
  • ns foo maps 'update to #'clojure.core/update
  • ns foo interns 'update, the compiler emits a warning
  • ns foo now maps 'update to #'foo/update
  • ns foo is reloaded
  • ns foo tries to map 'update to #'clojure.core/update but it's already mapped to #'foo/update

The logic in clojure.lang.Namespace/warnOnReplace makes it so that shadowing a clojure.core Var produces a warning while shadowing a Var from another namespace produces an error, this is what happening after reloading the namespace.

I haven't looked into the core.matrix code but I highly suspect that's what's going on there.

Comment by Alex Miller [ 01/Nov/14 11:27 AM ]

Definitely interested in a patch for this for the special case of clojure.core.

Comment by Nicola Mometto [ 01/Nov/14 11:41 AM ]

The attached patch fixes this issue by making warnOrFailOnReplace silently ignore when a clojure.core Var shadows another Var, which should only happen on namespace reload.

Comment by Mike Anderson [ 03/Nov/14 12:29 AM ]

The simplest way I can find to reproduce the general issue at the REPL is as follows:

(ns foo)
(def inc dec) ;; gets a warning
(ns bar (:require [foo :refer :all])) ;; gets another warning
(ns bar (:require [foo :refer :all])) ;; causes the exception (effectively a ns reload)

Preventing the exception is the biggest priority, it would be really nice to be also suppress the warnings. There are often good reasons to re-use names in clojure.core so it shouldn't cause a non-suppressible warning.

Note that the Clojure library coding standards say "Use good names, and don't be afraid to collide with names in other namespaces" so it is very inconsistent to trigger warnings / exceptions when people do exactly this.





[CLJ-1577] Some hints accept both symbols and class objects, others only symbols Created: 30/Oct/14  Updated: 30/Oct/14

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

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: typehints


 Description   

In order to hint primitives, such as longs, you can hint with the symbol 'long. In some places, you can also use the class object java.lang.Long/TYPE. However, in some places, that doesn't work. This is particularly problematic when working with hints in macros, where subtle changes to when metadata is evaluated can lead to changes in whether or not hints are respected.

user=> (set! *unchecked-math* :warn-on-boxed)
:warn-on-boxed

user=> (defmacro mac []
         (let [field (with-meta 'x {:tag 'long})]
           (-> field meta :tag class prn)
           `(deftype Foo# [~field]
              clojure.lang.IDeref
              (deref [this#]
                (inc ~(with-meta field nil))))))
#'user/mac

user=> (mac)
clojure.lang.Symbol
#<java.lang.Class@1c76c583 class user.Foo__13651__auto__>

user=> (defmacro mac []
         (let [field (with-meta 'x {:tag java.lang.Long/TYPE})]
           (-> field meta :tag class prn)
           `(deftype Foo# [~field]
              clojure.lang.IDeref
              (deref [this#]
                (inc ~(with-meta field nil))))))
#'user/mac

user=> (mac)
java.lang.Class
Boxed math warning, /private/var/folders/43/mnwlkd2s7r1gbjwq6t__mgt40000gn/T/form-init5463347341158437534.clj:1:1 - call: public static java.lang.Number clojure.lang.Numbers.unchecked_inc(java.lang.Object).
#<java.lang.Class@74626b21 class user.Foo__13663__auto__>





[CLJ-1576] clojure.pprint should print vars as pr does Created: 29/Oct/14  Updated: 28/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: print

Attachments: Text File pprint-vars-as-prn-does-0.patch    
Patch: Code
Approval: Triaged

 Description   

clojure.pprint/pprint currently by default lets vars fall through to its IDeref printing which prints them as something like:

#<Var@107e78: #<core$inc clojure.core$inc@f278dd>>

which is not a super representation of a var. vars have names.

generally when I pprint a data structure containing vars it is because at some point in writing the code that constructed that data structure I decided I wanted a history of the functions called, and since vars are invokable as functions and have a name, I can just use those as the history. the history then turns in to a big structure so I pretty print it, which then doesn't print the vars.

it is possible to work/around change the behaviour of the pretty printer by using its customizing options, but it is not a simple change to make, and means that for a small program a large percentage of it is spent making the pretty printer print something useful for vars.



 Comments   
Comment by Chris Blom [ 13/Nov/14 4:25 AM ]

I've added a patch which adds a method for clojure.lang.Var to
simple-dispatch in src/clojure/pprint/dispatch.clj:

(use-method simple-dispatch clojure.lang.Var pr)

The patch includes a simple test.

Comment by Aspasia Beneti [ 13/Nov/14 4:37 AM ]

Related bug http://dev.clojure.org/jira/browse/CLJ-1565





[CLJ-1575] Using a (def ^:const instance) of a deftype that implements IPersistentCollection, triggers compiler errors Created: 29/Oct/14  Updated: 30/Oct/14

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

Type: Defect Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

fresh repl


Attachments: Text File 0001-Test-for-analyzer-bug-CLJ-1575.patch    

 Description   

The compiler has a lot of assumptions about the possible types of IPersistentCollection literals and rightfully so. The strange thing with this case is, that taking the (constant) value works as soon as count is defined, but using it as an argument hits a closed dispatch for emitting the empty variants of the various literals.

> (deftype T [] clojure.lang.IPersistentCollection (count [_] 0)
> (def ^:const t (T.))
> (meta t)
java.lang.UnsupportedOperationException: Unknown Collection type
Compiler.java:2860 clojure.lang.Compiler$EmptyExpr.emit
Compiler.java:3632 clojure.lang.Compiler$InvokeExpr.emitArgsAndCall
...

EDIT updated the ticket after some investigation
NOTE attached test patch doesn't even implement (count []) for the deftype, which just triggers a rightful AbstractMethodError



 Comments   
Comment by Herwig Hochleitner [ 29/Oct/14 10:00 PM ]

The test had a typo, sorry

Comment by Alex Miller [ 30/Oct/14 7:14 AM ]

Looks like a variant of CLJ-1093.





[CLJ-1574] Vars defined in wrong namespace if ns form is not top-level Created: 28/Oct/14  Updated: 28/Oct/14  Resolved: 28/Oct/14

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

Type: Defect Priority: Major
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

I have a macro that given some file containing some data model description generates an API for accessing instances of that data model. That's the scenario although it's not really relevant. I've tracked down the issue to this minimal example.

This does the right thing:

;; in namespace user
(do (ns myns1)
    (defn myns1-fn [] nil)
    (in-ns 'user))

A new namespace myns1 is created containing one var myns1-fn. Now I can call (myns1/myns1-fn) and get 1.

However, the following does not work correctly:

;; in namespace user
(when-not (find-ns 'myns2)
  (do (ns myns2)
    (defn myns2-fn [] nil)
    (in-ns 'user)))

My intention is not to re-create the namespace myns2 in case it already exists. However, the result after the first evaluation (where myns2 doesn't exist yet) is that a new namespace myns2 is created, but the var myns2-fn is created in the user namespace (or whatever the current namespace is).

I know that `do` has some special casing to allow the first example. And the second example has an `if` at the top-level, so that's probably why it doesn't work. But it seems like a legit thing to do to test if a namespace exists, and if not, define it. E.g., you might have some optional dependency, and if it's not fulfilled, you just define the vars that you need yourself.



 Comments   
Comment by Nicola Mometto [ 28/Oct/14 6:15 AM ]

You can do what you are asking for by using intern rather than def

Comment by Alex Miller [ 28/Oct/14 9:02 AM ]

Something like this should work:

(when-not (find-ns 'myns2)
  (create-ns 'myns2)
  (intern 'myns2 'myns2-fn (fn [] "hello")))
Comment by Tassilo Horn [ 28/Oct/14 10:17 AM ]

Thanks Nicola and Alex. Using `intern` and `create-ns` is probably the better approach as it works in both cases.

But shouldn't that be somehow visible from the docs? Currently, `ns` says it changes the current value of `ns`, and `def` says it defines a var in the current namespace (`ns`). That leaves the impression that the second example is valid.

So maybe the docs of `def` and `ns` should contain a sentence like "If you want to create namespaces/Vars dynamically, prefer using `create-ns`/`intern` over `ns`/`def`."

Comment by Nicola Mometto [ 28/Oct/14 10:28 AM ]

Tassillo, I don't think ns is problematic here.
The issue is that def interns the var at compile time rather than at runtime and thus uses the compile time value of ns rather than the runtime one.

Comment by Tassilo Horn [ 28/Oct/14 2:48 PM ]

Thanks for the clarification, Nicola.





[CLJ-1573] Support (Java) transient fields in deftype, e.g. for hashcodes Created: 26/Oct/14  Updated: 29/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: compiler, deftype

Attachments: Text File 0001-transient-field-deftype.patch    
Patch: Code and Test

 Description   

Enhance deftypes to allow fields to be marked ACC_TRANSIENT.

strawman syntax:
(deftype AType [^:transient hash])

Came across this need while experimenting with a reified range written in a deftype, not in Java.

Patch doesn't include docstring change, but has a test.



 Comments   
Comment by Adrian Medina [ 29/Dec/14 11:54 AM ]

Perhaps ^:transient-mutable would be a more appropriate modifier name to be consistent with the ^:unsynchronized-mutable and ^:volatile-mutable field modifiers. In any event, this feature would eliminate the need to drop down to Java for types that require transient fields.

Comment by Andy Fingerhut [ 29/Dec/14 12:07 PM ]

Roberto, there is a "Vote" word you can click on to actually vote for tickets, and ticket wranglers actually look at those votes at times to examine popular ones sooner. +1 comments don't do that.





[CLJ-1572] into does not work with IReduceInit Created: 24/Oct/14  Updated: 10/Jan/15  Resolved: 10/Jan/15

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: transducers

Attachments: Text File clj-1572-2.patch     Text File clj-1572-3.patch     Text File clj-1572-4.patch     Text File CLJ-1572-alternative-POC.patch