<< Back to previous view

[CLJ-1868] Unknown return type class throws NPE instead of useful exception Created: 17/Dec/15  Updated: 08/Jan/16  Resolved: 08/Jan/16

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: compiler, errormsgs, regression

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

 Description   

This is a regression from CLJ-1232 - if you specify a return type class that is not fully-qualified or imported, you now get an NPE instead of a useful error message.

;; 1.7
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: Closeable, compiling:(NO_SOURCE_PATH:4:1)

;; 1.8
(defn foo ^Closeable [])
NullPointerException   clojure.core/sigs/resolve-tag--4375 (core.clj:247)

Cause: The new code that resolves classes does not handle the possible null return value of Compiler$HostExpr/maybeClass.

Solution: Check for null and fall back to the original argvecs, which will result in the original message.

Patch: clj-1868.patch



 Comments   
Comment by Nicola Mometto [ 17/Dec/15 5:10 PM ]

Dupe of CLJ-1863





[CLJ-1861] Remove unnecessary var interning Created: 02/Dec/15  Updated: 16/Dec/15  Resolved: 16/Dec/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Rich Hickey
Resolution: Completed Votes: 1
Labels: compiler

Approval: Vetted

 Description   

Remove unused var interns in static initializer (see https://groups.google.com/d/msg/clojure/731p1NBy2wk/lx98zp9oAAAJ ):

L1 {
             ldc "clojure.core" (java.lang.String)
             ldc "float?" (java.lang.String)
             invokestatic clojure/lang/RT var((Ljava/lang/String;Ljava/lang/String;)Lclojure/lang/Var;);
             checkcast clojure/lang/Var
             putstatic malt/utils$string_to_double.const__0:clojure.lang.Var
             ...
}


 Comments   
Comment by Nicola Mometto [ 02/Dec/15 9:49 AM ]

the compiler emits a lot of unused bytecode in its static initializer, not only vars.
The constant table is also full of unused numbers/keywords

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

resolved by Rich directly in https://github.com/clojure/clojure/commit/bfe14aec1c223abc3253358bac34b503284467d9

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

1.8.0-RC4





[CLJ-1856] Direct-linking breaks clojure.test do-report stack-depth assumptions for failure reporting. Created: 24/Nov/15  Updated: 05/Dec/15  Resolved: 05/Dec/15

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

Type: Defect Priority: Major
Reporter: Gary Trakhman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: regression, test

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

 Description   

Test failure locations are being mis-reported (wrong class/line number).

Given a test ns:

(ns test1856.core-test
  (:require [clojure.test :refer :all]
            [test1856.core :refer :all]))

(deftest fail-test
  ;; 6
  ;; 7
  (is (= nil true)))  ;; 8

The results with 1.8-RC2 (with CLJ-1854 patch included) are:

FAIL in (fail-test) (test.clj:342)    ;; WRONG, expected: (core_test.clj:8)
expected: (= nil true)
  actual: (not (= nil true))

Cause: The location of the error is calculated in test.clj by constructing a Throwable in do-report and then dropping the top 1 frame (which is do-report itself) to find the user frame where the assertion failed. However, with direct linking there are now 2 frames on top of of the failure in user code, so the same (incorrect) location in test.clj is reported every time.

Approach: Change to a different strategy to filter the top frames based on content, not hard-coded depth. This strategy will work regardless of whether direct linking is involved or not.

1. Instead of constructing an exception and using it's stack trace, instead call Thread.getStackTrace() on the current thread. Create a new function that works on stack traces rather than exceptions and no longer needs a depth check.
2. Drop top frames while their class name starts with java.lang (this filters the call to java.lang.Thread.getStackTrace()) or clojure.test. The top frame will then be in the user's code.
3. Deprecated old file-and-line function (not sure if other clojure test reporting frameworks use this, despite it being private).
4. Updated tests that check that these functions work with an empty stack trace, as the JVM may elide it.

Patch: clj-1856.patch



 Comments   
Comment by Gary Trakhman [ 24/Nov/15 4:35 PM ]

'2' works in the standard case of direct-linked clojure with non-direct-linked app code. I'm exploring if there's a way to get the line info via macro-expansion of 'try-expr' and passing the line value into do-report for those cases.

Comment by Gary Trakhman [ 24/Nov/15 4:54 PM ]

I altered a local clojure build to print the stacktrace of the Throwable created by do-report, showing the additional invokeStatic frames during a repl session, showing the first user function frame is at offset 2.

user> (use 'clojure.test)
nil
user> (deftest a []
        (is false))
#'user/a
user> (run-tests)

Testing user

FAIL in (a) (test.clj:342)
expected: false
  actual: false

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


java.lang.Throwable
    at clojure.test$do_report.invokeStatic(test.clj:355)
    at clojure.test$do_report.invoke(test.clj:342)
    at user$fn__16867.invokeStatic(NO_SOURCE_FILE:74)
    at user$fn__16867.invoke(NO_SOURCE_FILE:73)
    at clojure.test$test_var$fn__7973.invoke(test.clj:706)
    at clojure.test$test_var.invokeStatic(test.clj:706)
    at clojure.test$test_var.invoke(test.clj:697)
    at clojure.test$test_vars$fn__7995$fn__8000.invoke(test.clj:724)
    at clojure.test$default_fixture.invokeStatic(test.clj:676)
    at clojure.test$default_fixture.invoke(test.clj:672)
    at clojure.test$test_vars$fn__7995.invoke(test.clj:724)
    at clojure.test$default_fixture.invokeStatic(test.clj:676)
    at clojure.test$default_fixture.invoke(test.clj:672)
    at clojure.test$test_vars.invokeStatic(test.clj:720)
    at clojure.test$test_all_vars.invokeStatic(test.clj:726)
    at clojure.test$test_ns.invokeStatic(test.clj:747)
    at clojure.test$test_ns.invoke(test.clj:732)
    at clojure.core$map$fn__4781.invoke(core.clj:2648)
    at clojure.lang.LazySeq.sval(LazySeq.java:40)
    at clojure.lang.LazySeq.seq(LazySeq.java:49)
    at clojure.lang.Cons.next(Cons.java:39)
    at clojure.lang.RT.boundedLength(RT.java:1749)
    at clojure.lang.RestFn.applyTo(RestFn.java:130)
    at clojure.core$apply.invokeStatic(core.clj:647)
    at clojure.test$run_tests.invokeStatic(test.clj:757)
    at clojure.test$run_tests.doInvoke(test.clj:757)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.test$run_tests.invokeStatic(test.clj:762)
    at clojure.test$run_tests.invoke(test.clj:757)
    at user$eval16871.invokeStatic(NO_SOURCE_FILE:76)
    at user$eval16871.invoke(NO_SOURCE_FILE:76)
    at clojure.lang.Compiler.eval(Compiler.java:6915)
    at clojure.lang.Compiler.eval(Compiler.java:6878)
    at clojure.core$eval.invokeStatic(core.clj:3107)
    at clojure.core$eval.invoke(core.clj:3103)
    at clojure.main$repl$read_eval_print__7402$fn__7405.invoke(main.clj:240)
    at clojure.main$repl$read_eval_print__7402.invoke(main.clj:240)
    at clojure.main$repl$fn__7411.invoke(main.clj:258)
    at clojure.main$repl.invokeStatic(main.clj:258)
    at clojure.main$repl.doInvoke(main.clj:174)
    at clojure.lang.RestFn.invoke(RestFn.java:1523)
    at clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__10251.invoke(interruptible_eval.clj:87)
    at clojure.lang.AFn.applyToHelper(AFn.java:152)
    at clojure.lang.AFn.applyTo(AFn.java:144)
    at clojure.core$apply.invokeStatic(core.clj:645)
    at clojure.core$with_bindings_STAR_.invokeStatic(core.clj:1883)
    at clojure.core$with_bindings_STAR_.doInvoke(core.clj:1883)
    at clojure.lang.RestFn.invoke(RestFn.java:425)
    at clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invokeStatic(interruptible_eval.clj:85)
    at clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invoke(interruptible_eval.clj:55)
    at clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn__10296$fn__10299.invoke(interruptible_eval.clj:222)
    at clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__10291.invoke(interruptible_eval.clj:190)
    at clojure.lang.AFn.run(AFn.java:22)
    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)

This is also with the patch from http://dev.clojure.org/jira/browse/CLJ-1854 applied

Comment by Alex Miller [ 25/Nov/15 1:26 PM ]
(ns test1856.core-test
  (:require [clojure.test :refer :all]
            [test1856.core :refer :all]))

(deftest throw-test
  ;; 6
  ;; 7
  (is (= nil (throw (Exception. "abc")))))  ;; 8

(deftest fail-test
  ;; 11
  ;; 12
  (is (= nil true)))  ;; 13

If I lein test (or run from repl), I see:

;; 1.7
FAIL in (fail-test) (core_test.clj:13)
expected: (= nil true)
  actual: (not (= nil true))

ERROR in (throw-test) (core_test.clj:8)
expected: (= nil (throw (Exception. "abc")))
  actual: java.lang.Exception: abc
 at test1856.core_test/fn (core_test.clj:8)
    clojure.test$test_var$fn__7670.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)
    clojure.test$test_vars$fn__7692$fn__7697.invoke (test.clj:722)
    ...
;; 1.8.0-RC2 + CLJ-1854 patch
FAIL in (fail-test) (test.clj:342)    ;; ERROR
expected: (= nil true)
  actual: (not (= nil true))

ERROR in (throw-test) (core_test.clj:8)
expected: (= nil (throw (Exception. "abc")))
  actual: java.lang.Exception: abc
 at test1856.core_test$fn__201.invokeStatic (core_test.clj:8)  ;; OK
    test1856.core_test/fn (core_test.clj:5)
    clojure.test$test_var$fn__7972.invoke (test.clj:703)
    clojure.test$test_var.invokeStatic (test.clj:703)




[CLJ-1854] Direct-linking changes lose line-number on invoke() Created: 24/Nov/15  Updated: 01/Dec/15  Resolved: 01/Dec/15

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

Type: Defect Priority: Major
Reporter: Gary Trakhman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: compiler, regression
Environment:

Clojure 1.8RC2, leiningen 2.5.1


Attachments: Text File CLJ-1854-more-context.patch     Text File CLJ-1854.patch    
Patch: Code
Approval: Ok

 Description   
(ns foo)  ;; 1
;; 2
;; 3
;; 4
;; 5
;; 6
;; 7
(defn callstack []                       ;; 8
  [1 2 3]                                ;; 9
  (throw (Exception. "whoopsie!")))      ;; 10

Stack Trace comparison. Only the first two lines of each trace are relevant, the rest is all REPL fluff.

;;; 1.7.0
{:cause "whoopsie!",
 :via
 [{:type java.lang.Exception,
   :message "whoopsie!",
   :at [foo$callstack invoke "foo.clj" 8]}],
 :trace
 [[foo$callstack invoke "foo.clj" 8]
  [user$eval7675 invoke "form-init3342294504880003721.clj" 1]
  [clojure.lang.Compiler eval "Compiler.java" 6782]
  [clojure.lang.Compiler eval "Compiler.java" 6745]
	...

;;; 1.8 RC2
{:cause "whoopsie!",
 :via
 [{:type java.lang.Exception,
   :message "whoopsie!",
   :at [foo$callstack invokeStatic "foo.clj" 8]}],
 :trace
 [[foo$callstack invokeStatic "foo.clj" 8]
  [foo$callstack invoke "foo.clj" -1]    ;; Unexpected: -1
  [user$eval4 invokeStatic "NO_SOURCE_FILE" 28]
  [user$eval4 invoke "NO_SOURCE_FILE" -1]    ;; Unexpected: -1
  [clojure.lang.Compiler eval "Compiler.java" 6913]
  [clojure.lang.Compiler eval "Compiler.java" 6876]
	...

;;; 1.8 RC2 with patch
{:cause "whoopsie!",
 :via
 [{:type java.lang.Exception,
   :message "whoopsie!",
   :at [foo$callstack invokeStatic "foo.clj" 8]}],
 :trace
 [[foo$callstack invokeStatic "foo.clj" 8]
  [foo$callstack invoke "foo.clj" 8]    ;; Fixed
  [user$eval4 invokeStatic "NO_SOURCE_FILE" 3]
  [user$eval4 invoke "NO_SOURCE_FILE" 3]   ;; Fixed
  [clojure.lang.Compiler eval "Compiler.java" 6917]
  [clojure.lang.Compiler eval "Compiler.java" 6880]
	...

Cause: Non-direct linking now calls from invoke() through to invokeStatic(). In invoke(), Compiler does not visitLineNumber() before invoke() calls invokeStatic(), meaning that stack traces end up with -1 instead of a useful line number.

Patch: CLJ-1854-more-context.patch

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 24/Nov/15 1:18 PM ]

I have tested with Clojure 1.8.0-RC2 plus patch CLJ-1854.patch, and it does cause all of the -1 line numbers I have seen in stack traces to be filled in with actual source code line numbers.

For an example see this output after the patch: https://github.com/jafingerhut/st/blob/master/out-clj18-rc2-plus-clj1854-patch.txt

compared to this output with unmodified Clojure 1.8.0-RC2: https://github.com/jafingerhut/st/blob/master/out-clj18-rc2.txt

Comment by Alex Miller [ 24/Nov/15 1:30 PM ]

Ghadi, can you re-make the patch with more lines of diff context (use -U15 on format-patch)?

Comment by Ghadi Shayban [ 24/Nov/15 1:54 PM ]

np. -U15 wasn't enough, used -U30

Comment by Alex Miller [ 24/Nov/15 2:18 PM ]

Does it make sense for the two frames for the invoke and invokeStatic to refer to different line numbers in the source?

Comment by Ghadi Shayban [ 24/Nov/15 3:44 PM ]

Example has recursion through walk and is not minimal. Editing the ticket for reproducibility.

Comment by Gary Trakhman [ 24/Nov/15 3:47 PM ]

The current CLJ-1854-more-context.patch just gives me the same line number for all test cases, in my case it's test.clj:342 instead of -1 from before. I think perhaps clojure.test might need an adjustment as well, in particular the hardcoded '1' magic number here: https://github.com/clojure/clojure/blob/master/src/clj/clojure/test.clj#L351

test.clj:342 is do-report: https://github.com/clojure/clojure/blob/master/src/clj/clojure/test.clj#L342

Comment by Alex Miller [ 24/Nov/15 3:56 PM ]

Gary Trakhman I think that issue should be a separate ticket if so.

Comment by Gary Trakhman [ 24/Nov/15 4:03 PM ]

I will make a separate ticket for the potential clojure.test change.

Comment by Gary Trakhman [ 24/Nov/15 5:20 PM ]

Comparison of line numbers between 1.7 and 1.8 with patch here applied, clojure.test/do-report was modified to print stacktraces. It's weird that the numbers are different between parallel invoke/invokeStatic pairs.

diff --git a/src/clj/clojure/test.clj b/src/clj/clojure/test.clj
index 55e00f7..318ef20 100644
--- a/src/clj/clojure/test.clj
+++ b/src/clj/clojure/test.clj
@@ -349,7 +349,10 @@
   (report
    (case
     (:type m)
-    :fail (merge (file-and-line (new java.lang.Throwable) 1) m)
+     :fail (merge (file-and-line (doto (new java.lang.Throwable)
+                                   (.printStackTrace))
+                                 1)
+                  m)
     :error (merge (file-and-line (:actual m) 0) m) 
     m)))

1.7

gary@gary-dell:~/.m2/repository/org/clojure/clojure/1.7.0$ java -jar clojure-1.7.0.jar -r
Clojure 1.7.0
user=> (use 'clojure.test)
nil
user=> (deftest a []
(is false))
#'user/a
user=> (run-tests)

Testing user
java.lang.Throwable
    at clojure.test$do_report.invoke(test.clj:352)
    at user$fn__3.invoke(NO_SOURCE_FILE:3)
    at clojure.test$test_var$fn__7671.invoke(test.clj:707)
    at clojure.test$test_var.invoke(test.clj:707)
    at clojure.test$test_vars$fn__7693$fn__7698.invoke(test.clj:725)
    at clojure.test$default_fixture.invoke(test.clj:677)
    at clojure.test$test_vars$fn__7693.invoke(test.clj:725)
    at clojure.test$default_fixture.invoke(test.clj:677)
    at clojure.test$test_vars.invoke(test.clj:721)
    at clojure.test$test_all_vars.invoke(test.clj:731)
    at clojure.test$test_ns.invoke(test.clj:750)
    at clojure.core$map$fn__4553.invoke(core.clj:2624)
    at clojure.lang.LazySeq.sval(LazySeq.java:40)
    at clojure.lang.LazySeq.seq(LazySeq.java:49)
    at clojure.lang.Cons.next(Cons.java:39)
    at clojure.lang.RT.boundedLength(RT.java:1735)
    at clojure.lang.RestFn.applyTo(RestFn.java:130)
    at clojure.core$apply.invoke(core.clj:632)
    at clojure.test$run_tests.doInvoke(test.clj:765)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.test$run_tests.invoke(test.clj:763)
    at user$eval7.invoke(NO_SOURCE_FILE:4)
    at clojure.lang.Compiler.eval(Compiler.java:6782)
    at clojure.lang.Compiler.eval(Compiler.java:6745)
    at clojure.core$eval.invoke(core.clj:3081)
    at clojure.main$repl$read_eval_print__7099$fn__7102.invoke(main.clj:240)
    at clojure.main$repl$read_eval_print__7099.invoke(main.clj:240)
    at clojure.main$repl$fn__7108.invoke(main.clj:258)
    at clojure.main$repl.doInvoke(main.clj:258)
    at clojure.lang.RestFn.invoke(RestFn.java:421)
    at clojure.main$repl_opt.invoke(main.clj:324)
    at clojure.main$main.doInvoke(main.clj:421)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.lang.Var.invoke(Var.java:379)
    at clojure.lang.AFn.applyToHelper(AFn.java:154)
    at clojure.lang.Var.applyTo(Var.java:700)
    at clojure.main.main(main.java:37)

FAIL in (a) (NO_SOURCE_FILE:3)
expected: false
  actual: false

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

1.8 with patch here applied

gary@gary-dell:~/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT$ java -jar clojure-1.8.0-master-SNAPSHOT.jar -r
Clojure 1.8.0-master-SNAPSHOT
user=> (use 'clojure.test)
nil
user=> (deftest a []
(is false))
#'user/a
user=> (run-tests)

Testing user
java.lang.Throwable
    at clojure.test$do_report.invokeStatic(test.clj:355)
    at clojure.test$do_report.invoke(test.clj:342)
    at user$fn__3.invokeStatic(NO_SOURCE_FILE:3)
    at user$fn__3.invoke(NO_SOURCE_FILE:2)
    at clojure.test$test_var$fn__7973.invoke(test.clj:706)
    at clojure.test$test_var.invokeStatic(test.clj:706)
    at clojure.test$test_var.invoke(test.clj:697)
    at clojure.test$test_vars$fn__7995$fn__8000.invoke(test.clj:724)
    at clojure.test$default_fixture.invokeStatic(test.clj:676)
    at clojure.test$default_fixture.invoke(test.clj:672)
    at clojure.test$test_vars$fn__7995.invoke(test.clj:724)
    at clojure.test$default_fixture.invokeStatic(test.clj:676)
    at clojure.test$default_fixture.invoke(test.clj:672)
    at clojure.test$test_vars.invokeStatic(test.clj:720)
    at clojure.test$test_all_vars.invokeStatic(test.clj:726)
    at clojure.test$test_ns.invokeStatic(test.clj:747)
    at clojure.test$test_ns.invoke(test.clj:732)
    at clojure.core$map$fn__4781.invoke(core.clj:2648)
    at clojure.lang.LazySeq.sval(LazySeq.java:40)
    at clojure.lang.LazySeq.seq(LazySeq.java:49)
    at clojure.lang.Cons.next(Cons.java:39)
    at clojure.lang.RT.boundedLength(RT.java:1749)
    at clojure.lang.RestFn.applyTo(RestFn.java:130)
    at clojure.core$apply.invokeStatic(core.clj:647)
    at clojure.test$run_tests.invokeStatic(test.clj:757)
    at clojure.test$run_tests.doInvoke(test.clj:757)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.test$run_tests.invokeStatic(test.clj:762)
    at clojure.test$run_tests.invoke(test.clj:757)
    at user$eval7.invokeStatic(NO_SOURCE_FILE:4)
    at user$eval7.invoke(NO_SOURCE_FILE:4)
    at clojure.lang.Compiler.eval(Compiler.java:6915)
    at clojure.lang.Compiler.eval(Compiler.java:6878)
    at clojure.core$eval.invokeStatic(core.clj:3107)
    at clojure.core$eval.invoke(core.clj:3103)
    at clojure.main$repl$read_eval_print__7402$fn__7405.invoke(main.clj:240)
    at clojure.main$repl$read_eval_print__7402.invoke(main.clj:240)
    at clojure.main$repl$fn__7411.invoke(main.clj:258)
    at clojure.main$repl.invokeStatic(main.clj:258)
    at clojure.main$repl_opt.invokeStatic(main.clj:322)
    at clojure.main$repl_opt.invoke(main.clj:318)
    at clojure.main$main.invokeStatic(main.clj:421)
    at clojure.main$main.doInvoke(main.clj:384)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.lang.Var.invoke(Var.java:379)
    at clojure.lang.AFn.applyToHelper(AFn.java:154)
    at clojure.lang.Var.applyTo(Var.java:700)
    at clojure.main.main(main.java:37)

FAIL in (a) (test.clj:342)
expected: false
  actual: false

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




[CLJ-1853] Socket server can't use user-defined accept-fns Created: 24/Nov/15  Updated: 01/Dec/15  Resolved: 01/Dec/15

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

Type: Defect Priority: Major
Reporter: OHTA Shogo Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: server

Attachments: Text File 0001-CLJ-1853-Require-the-ns-before-resolving-the-accept-.patch    
Patch: Code
Approval: Ok

 Description   

In 1.8.0 RC2, if you start a socket server with a user-defined accept-fn like the following (clojure.test/testing-contexts-str is just a 0-arity fn used as an example accept fn here):

$ java -cp clojure-1.8.0-RC2.jar -Dclojure.server.foo='{:port 5555 :accept clojure.test/testing-contexts-str}' clojure.main

And then, if you connect to it with a command like "telnet 127.0.0.1 5555", you'll get an NPE.

Clojure 1.8.0-RC2
user=> Exception in thread "Clojure Connection repl 1" java.lang.NullPointerException
        at clojure.core$apply.invokeStatic(core.clj:645)
        at clojure.core.server$accept_connection.invokeStatic(server.clj:60)
        at clojure.core.server$start_server$fn__7327$fn__7328$fn__7330.invoke(server.clj:104)
        at clojure.lang.AFn.run(AFn.java:22)
        at java.lang.Thread.run(Thread.java:745)

This doesn't happen when starting the server with a pre-defined accept-fn, such as clojure.core.server/repl.

Cause: At the moment, clojure.core.server/accept-connection will require the namespace in which the accept-fn is defined after resolving the accept-fn. However, in order to resolve the accept-fn successfully, requiring the ns should be done prior to it.

Approach: Requiring the ns before resolving the accept-fn.

Patch: 0001-CLJ-1853-Require-the-ns-before-resolving-the-accept-.patch

Screened by: Alex Miller






[CLJ-1851] Add :redef key for vars to avoid being direct linked Created: 20/Nov/15  Updated: 05/Dec/15  Resolved: 05/Dec/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

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

 Description   

It is useful in some cases to indicate that calls to a var should never be direct linked. That is possible with ^:dynamic but that has additional semantics (and cost). Add a new ^:redef meta for vars that prevents direct invocations but does not have the ^:dynamic semantics.

From CLJ-1845, load was marked dynamic for this reason, now change to redef instead.

Patch: clj-1851.patch (also changes load to be :redef rather than :dynamic)






[CLJ-1849] Add tests for CLJ-1846 and CLJ-1825 Created: 13/Nov/15  Updated: 16/Nov/15  Resolved: 16/Nov/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

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

 Description   

Add tests to reproduce CLJ-1846 and CLJ-1825 errors for future testing.






[CLJ-1846] VerifyError in Clojure 1.8.0-(beta1..RC1) Created: 11/Nov/15  Updated: 11/Nov/15  Resolved: 11/Nov/15

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

Type: Defect Priority: Major
Reporter: Shantanu Kumar Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: compiler, regression
Environment:

Oracle JDK 1.7, Oracle JDK 1.8 on Mac OS X


Approval: Vetted

 Description   

Nicola Mometto provided the below minimal repro case:

user=> (defn foo ^long [] 1)
#'user/foo
user=> (Integer/bitCount ^int (foo))
VerifyError (class: user$eval13, method: invokeStatic signature: ()Ljava/lang/Object;) Expecting to find integer on stack  java.lang.Class.getDeclaredConstructors0 (Class.java:-2)

Full stack trace as found with https://github.com/kumarshantanu/asphalt:

$ lein do clean, with-profile dev,c18 test
Exception in thread "main" java.lang.VerifyError: (class: asphalt/core$invoke_with_transaction, method: invokeStatic signature: (Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;) Expecting to find integer on stack, compiling:(core.clj:201:1)
	at clojure.lang.Compiler$DefExpr.eval(Compiler.java:463)
	at clojure.lang.Compiler.eval(Compiler.java:6918)
	at clojure.lang.Compiler.load(Compiler.java:7360)
	at clojure.lang.RT.loadResourceScript(RT.java:372)
	at clojure.lang.RT.loadResourceScript(RT.java:363)
	at clojure.lang.RT.load(RT.java:453)
	at clojure.lang.RT.load(RT.java:419)
	at clojure.core$load$fn__5673.invoke(core.clj:5895)
	at clojure.core$load.invokeStatic(core.clj:5894)
	at clojure.core$load_one.invokeStatic(core.clj:5694)
	at clojure.core$load_one.invoke(core.clj)
	at clojure.core$load_lib$fn__5622.invoke(core.clj:5739)
	at clojure.core$load_lib.invokeStatic(core.clj:5738)
	at clojure.core$load_lib.doInvoke(core.clj)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invokeStatic(core.clj:647)
	at clojure.core$load_libs.invokeStatic(core.clj:5776)
	at clojure.core$load_libs.doInvoke(core.clj)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invokeStatic(core.clj:647)
	at clojure.core$require.invokeStatic(core.clj:5798)
	at clojure.core$require.doInvoke(core.clj)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at asphalt.test_util$eval198$loading__5565__auto____199.invoke(test_util.clj:1)
	at asphalt.test_util$eval198.invokeStatic(test_util.clj:1)
	at asphalt.test_util$eval198.invoke(test_util.clj)
	at clojure.lang.Compiler.eval(Compiler.java:6913)
	at clojure.lang.Compiler.eval(Compiler.java:6902)
	at clojure.lang.Compiler.load(Compiler.java:7360)
	at clojure.lang.RT.loadResourceScript(RT.java:372)
	at clojure.lang.RT.loadResourceScript(RT.java:363)
	at clojure.lang.RT.load(RT.java:453)
	at clojure.lang.RT.load(RT.java:419)
	at clojure.core$load$fn__5673.invoke(core.clj:5895)
	at clojure.core$load.invokeStatic(core.clj:5894)
	at clojure.core$load_one.invokeStatic(core.clj:5694)
	at clojure.core$load_one.invoke(core.clj)
	at clojure.core$load_lib$fn__5622.invoke(core.clj:5739)
	at clojure.core$load_lib.invokeStatic(core.clj:5738)
	at clojure.core$load_lib.doInvoke(core.clj)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invokeStatic(core.clj:647)
	at clojure.core$load_libs.invokeStatic(core.clj:5776)
	at clojure.core$load_libs.doInvoke(core.clj)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invokeStatic(core.clj:647)
	at clojure.core$require.invokeStatic(core.clj:5798)
	at clojure.core$require.doInvoke(core.clj)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at asphalt.core_test$eval192$loading__5565__auto____193.invoke(core_test.clj:1)
	at asphalt.core_test$eval192.invokeStatic(core_test.clj:1)
	at asphalt.core_test$eval192.invoke(core_test.clj)
	at clojure.lang.Compiler.eval(Compiler.java:6913)
	at clojure.lang.Compiler.eval(Compiler.java:6902)
	at clojure.lang.Compiler.load(Compiler.java:7360)
	at clojure.lang.RT.loadResourceScript(RT.java:372)
	at clojure.lang.RT.loadResourceScript(RT.java:363)
	at clojure.lang.RT.load(RT.java:453)
	at clojure.lang.RT.load(RT.java:419)
	at clojure.core$load$fn__5673.invoke(core.clj:5895)
	at clojure.core$load.invokeStatic(core.clj:5894)
	at clojure.core$load_one.invokeStatic(core.clj:5694)
	at clojure.core$load_one.invoke(core.clj)
	at clojure.core$load_lib$fn__5622.invoke(core.clj:5739)
	at clojure.core$load_lib.invokeStatic(core.clj:5738)
	at clojure.core$load_lib.doInvoke(core.clj)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invokeStatic(core.clj:647)
	at clojure.core$load_libs.invokeStatic(core.clj:5776)
	at clojure.core$load_libs.doInvoke(core.clj)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invokeStatic(core.clj:647)
	at clojure.core$require.invokeStatic(core.clj:5798)
	at clojure.core$require.doInvoke(core.clj)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invokeStatic(core.clj:647)
	at clojure.core$apply.invoke(core.clj)
	at user$eval91.invokeStatic(form-init7505432955041312280.clj:1)
	at user$eval91.invoke(form-init7505432955041312280.clj)
	at clojure.lang.Compiler.eval(Compiler.java:6913)
	at clojure.lang.Compiler.eval(Compiler.java:6903)
	at clojure.lang.Compiler.load(Compiler.java:7360)
	at clojure.lang.Compiler.loadFile(Compiler.java:7298)
	at clojure.main$load_script.invokeStatic(main.clj:275)
	at clojure.main$init_opt.invokeStatic(main.clj:277)
	at clojure.main$init_opt.invoke(main.clj)
	at clojure.main$initialize.invokeStatic(main.clj:308)
	at clojure.main$null_opt.invokeStatic(main.clj:342)
	at clojure.main$null_opt.invoke(main.clj)
	at clojure.main$main.invokeStatic(main.clj:421)
	at clojure.main$main.doInvoke(main.clj)
	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.VerifyError: (class: asphalt/core$invoke_with_transaction, method: invokeStatic signature: (Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;) Expecting to find integer on stack
	at java.lang.Class.getDeclaredConstructors0(Native Method)
	at java.lang.Class.privateGetDeclaredConstructors(Class.java:2671)
	at java.lang.Class.getConstructor0(Class.java:3075)
	at java.lang.Class.newInstance(Class.java:412)
	at clojure.lang.Compiler$ObjExpr.eval(Compiler.java:4902)
	at clojure.lang.Compiler$DefExpr.eval(Compiler.java:450)
	... 95 more


 Comments   
Comment by Nicola Mometto [ 11/Nov/15 8:23 AM ]

Copying a comment I posted on the ML regarding this bug:

To be honest I'm not sure this should even be a valid use of type hints.
You're telling the compiler that the result of (foo) is an int, when it is infact a long.

The correct way to do this should be:

(Integer/bitCount (int (foo))

Again, lack of specification on what the correct type hinting semantics should be make it hard to evaluate if this should be considered a bug or just an user error that previously just got ignored.

Comment by Alex Miller [ 11/Nov/15 3:18 PM ]

The example Nicola gave in the description worked in 1.6 and 1.7 and 1.8 up to 1.8.0-alpha2. It started failing as of https://github.com/clojure/clojure/commit/8c9580cb6706f2dc40bb31bbdb96a6aefe341bd5 for CLJ-1533.

Comment by Alex Miller [ 11/Nov/15 3:20 PM ]

Rich pushed a new commit https://github.com/clojure/clojure/commit/9448d627e091bc010e68e05a5669c134cd715a98 for this in master.

Comment by Alex Miller [ 11/Nov/15 3:20 PM ]

The commit makes this kind of incorrect type hint (previously a no op) now a compile error.

Comment by Shantanu Kumar [ 11/Nov/15 8:59 PM ]

I tested with the latest master and it correctly reports the "Caused by: java.lang.UnsupportedOperationException: Cannot coerce long to int, use a cast instead" error now. However, the reported line number in the exception is that of the defn (first line of the fn) instead of where the coercion was attempted in the fn body.





[CLJ-1845] Allow load to be redefined Created: 10/Nov/15  Updated: 05/Dec/15  Resolved: 05/Dec/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1845.patch     Text File clj-1845-test.patch     Text File ctyp1845-direct-linking-test.patch    
Patch: Code
Approval: Ok

 Description   

With direct linking of core, we have lost the ability to easily redef load (as calls to load inside Clojure are direct linked).

Proposed: make load dynamic (dynamic vars are not direct linked)

Patch: clj-1845.patch
See: https://groups.google.com/d/msg/clojure/_AGdLHSg41Q/q8LeplkrBQAJ

------------------------------------------

Re-opened because the initial declare of load is not declared as ^:dynamic and thus functions that use load between the initial forward declare and the later actual declaration were still allowing direct linking.

Because we are adding ^:redef, I rolled the changes into CLJ-1851 instead. The only thing here is a new test (which will fail till CLJ-1851 is applied).

Test patch: clj-1845-test.patch (NEW)
See also: CLJ-1851
Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 20/Nov/15 9:18 AM ]

Reopening...

Comment by Alex Miller [ 20/Nov/15 9:19 AM ]

Test (that doesn't work):

user=> (alter-var-root #'load (fn [f] (fn [& args] (prn "patched") (apply f args))))
#object[user$eval1241$fn__1242$fn__1243 0x1c857e6 "user$eval1241$fn__1242$fn__1243@1c857e6"]
user=> (load)
"patched"
nil
user=> (require 'clojure.core :reload)   ;; expect to see "patched"
nil
user=> (require 'clojure.tools.analyzer :reload)   ;; expect to see "patched"
nil
user=> (require 'clojure.tools.analyzer :reload-all)   ;; expect to see "patched"
nil
Comment by Alex Miller [ 20/Nov/15 9:20 AM ]

The issue is that load is forward-declared and the forward declaration does not declare dynamic. Replacing (declare load) with (def ^:declared ^:dynamic load) will fix it.

Comment by Ambrose Bonnaire-Sergeant [ 20/Nov/15 9:47 AM ]

Interested in a patch with a test?

Comment by Ambrose Bonnaire-Sergeant [ 20/Nov/15 9:52 AM ]

Confirmed that (declare ^:dynamic load) fixes the problem

Comment by Alex Miller [ 20/Nov/15 9:55 AM ]

No patch - this interacts with another change and I may just roll them together.

Comment by Alex Miller [ 20/Nov/15 9:56 AM ]

Actually, if you wanted to make a patch for a test, that would be useful.

Comment by Ambrose Bonnaire-Sergeant [ 20/Nov/15 10:12 AM ]

Attached direct linking test.

Comment by Alex Miller [ 20/Nov/15 11:00 AM ]

New test patch that applies to master

Comment by Andy Fingerhut [ 05/Dec/15 3:13 PM ]

It appears that CLJ-1845, CLJ-1851, and CLJ-1856 are committed now, and can be closed as complete?





[CLJ-1834] Support for test matrix build of direct linking on / off Created: 26/Oct/15  Updated: 27/Oct/15  Resolved: 27/Oct/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: build

Attachments: Text File clj-1834-2.patch     Text File clj-1834-3.patch     Text File clj-1834.patch    
Patch: Code
Approval: Ok

 Description   

Enable build box test matrix build of direct linking on and off.

Modified build to do the following:

  • Maven build - add user property "directlinking" with default value "true"
  • Maven build - add two profiles: "direct" and "nodirect" which force this property to either "true" or "false"
  • Ant build - defines new property "directlinking"
  • Ant build - inherits this property value from Maven automatically
  • Ant build - echoes the setting of the property during test compilation so you can tell which is activated
  • Ant build - compiles and runs tests with clojure.compiler.direct-linking set to the value of ${directlinking}

The Maven build can be invoked with one of these as follows:

mvn clean test -Ptest-direct
mvn clean test -Ptest-no-direct

The Hudson clojure-test-matrix will have a new axis with values "test-direct" and "test-no-direct" and a modified command line that will set the profile with -P based on the axis value.

Patch: clj-1834-3.patch



 Comments   
Comment by Alex Miller [ 26/Oct/15 5:10 PM ]

I may have broken the default ant build behavior with this patch, need to check on that still but taking a break for now...

Comment by Alex Miller [ 27/Oct/15 9:03 AM ]

Ant behavior fixed in -2 patch





[CLJ-1831] Add map-entry? predicate Created: 19/Oct/15  Updated: 27/Oct/15  Resolved: 27/Oct/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

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

 Description   

Due to changes in 1.8 with making vector implement IMapEntry for 2-element vectors, checking whether something is a map entry has become a bit trickier. This enhancement proposes a new function `map-entry?` to encapsulate that check and any future updates to it.

The check for map-entry? will return true if the instance implements java.util.Map$Entry and if it is also a vector, if it's size is exactly 2.

Patch: clj-1831.patch

Screened by Joe Smith.



 Comments   
Comment by Nicola Mometto [ 24/Oct/15 3:33 PM ]

Joe R. Smith Only members of the clojure core team are allowed to screen tickets

Comment by Ghadi Shayban [ 24/Oct/15 3:39 PM ]

Nicola, Joe Smith is core team.

Comment by Nicola Mometto [ 24/Oct/15 5:21 PM ]

Sorry about that then, I restored the ticket status

Comment by Nicola Mometto [ 24/Oct/15 5:23 PM ]

http://dev.clojure.org/display/community/Screeners should probably be updated then

Comment by Alex Miller [ 24/Oct/15 9:08 PM ]

Yep, will do.





[CLJ-1829] VerifyError on Android Created: 16/Oct/15  Updated: 11/Jan/16  Resolved: 11/Jan/16

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

Type: Defect Priority: Major
Reporter: Konstantin Mikheev Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: android, compiler
Environment:

Android API >= 21


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

 Description   

Android Lollipop (API level 21) introduced an advanced bytecode checker that does not allow Clojure 1.8 to run.

Here is an example repo that reproduces the issue:
https://github.com/konmik/try_clojure_on_android/blob/master/app/src/main/java/com/clojure_on_android/TestInvokeUnit.java#L8

It uses 'org.clojure:clojure:1.8.0-beta1' dependency.

To reproduce the exception you need to install Android Studio
https://developer.android.com/sdk/index.html
and an android emulator https://www.genymotion.com/

Run the emulator, open the project and press "run" in the IDE.

The expected result that I get on Android API < 21 is:
https://github.com/konmik/try_clojure_on_android/blob/master/expected.png

On Android versions >= 21 I get:

10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: java.lang.VerifyError: Rejecting class clojure.core.server$stop_server because it failed compile-time verification (declaration of 'clojure.core.server$stop_server' appears in /data/app/com.clojure_on_android-1/base.apk)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core.server__init.load(Unknown Source)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core.server__init.<clinit>(Unknown Source)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at java.lang.Class.classForName(Native Method)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at java.lang.Class.forName(Class.java:309)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.lang.RT.classForName(RT.java:2168)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.lang.RT.classForName(RT.java:2177)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.lang.RT.loadClassForName(RT.java:2196)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.lang.RT.load(RT.java:443)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.lang.RT.load(RT.java:419)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core$load$fn__5669.invoke(core.clj:5885)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core$load.invokeStatic(core.clj:5884)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core$load_one.invokeStatic(core.clj:5685)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core$load_one.invoke(core.clj)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core$load_lib$fn__5618.invoke(core.clj:5730)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core$load_lib.invokeStatic(core.clj:5729)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core$load_lib.doInvoke(core.clj)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.lang.RestFn.applyTo(RestFn.java:142)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core$apply.invokeStatic(core.clj:647)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core$load_libs.invokeStatic(core.clj:5767)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core$load_libs.doInvoke(core.clj)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.lang.RestFn.applyTo(RestFn.java:137)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core$apply.invokeStatic(core.clj:647)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core$require.invokeStatic(core.clj:5789)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.core$require.doInvoke(core.clj)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.lang.RestFn.invoke(RestFn.java:408)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.lang.Var.invoke(Var.java:379)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.lang.RT.doInit(RT.java:480)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.lang.RT.<clinit>(RT.java:331)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.lang.Namespace.<init>(Namespace.java:34)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.lang.Namespace.findOrCreate(Namespace.java:176)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.lang.Var.intern(Var.java:146)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.java.api.Clojure.var(Clojure.java:82)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at clojure.java.api.Clojure.<clinit>(Clojure.java:96)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at com.clojure_on_android.TestInvokeUnit.invokePlus(TestInvokeUnit.java:8)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at com.clojure_on_android.MainActivity.onCreate(MainActivity.java:15)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at android.app.Activity.performCreate(Activity.java:5990)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1106)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2278)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2387)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at android.app.ActivityThread.access$800(ActivityThread.java:151)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1303)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at android.os.Handler.dispatchMessage(Handler.java:102)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at android.os.Looper.loop(Looper.java:135)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at android.app.ActivityThread.main(ActivityThread.java:5254)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at java.lang.reflect.Method.invoke(Native Method)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at java.lang.reflect.Method.invoke(Method.java:372)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:903)
10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:698)

Cause: In Clojure 1.8, the socket server code is loaded during startup, which causes classes to be loaded that are compiled with the locking macro. The locking macro generates monitorenter and monitorexit instructions (and exception handlers) that do not conform to the (optional) structured locking section of the JVM spec. While this code is considered valid in OracleJDK, OpenJDK, etc the new Android bytecode checker will fail with it. Other versions of Clojure also have this verification issue, but the use of the locking macro during language boot time changes this potential issue to always being a problem.

Approach: The proposed patch sidesteps this issue by avoiding the locking macro and replaces it with a similar macro that uses ReentrantLock instead. This approach has been verified on the provided test case.

Patch: clj-1829.patch

Alternatives: There is a patch available for the locking macro (CLJ-1472) that replaces the locking macro by a synchronized block instead.



 Comments   
Comment by Alex Miller [ 16/Oct/15 2:32 PM ]

This could be a result of CLJ-1809 (hard to tell). The clojure.core.server/stop-server fn is a new fn with the socket server feature and should be direct-linked, which could implicate 1809.

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

I tried to reproduce this (using Run -> Run 'build' in Android Studio). The build was successful. I suspect I'm missing one or more steps in how to run correctly. Do I need to add a virtual device in Genymotion and if so, does it matter which one?

Comment by Konstantin Mikheev [ 16/Oct/15 3:17 PM ]

Yes, the build is successful.

The issue appears when you run it on a device.

You need to add a device in the emulator with API level >= 21 and to run it there.

Comment by Konstantin Mikheev [ 16/Oct/15 3:20 PM ]

When you run it, the logcat (Android logging panel) appears that is showing you the system log. The application's crash log can be found there.

Comment by Alex Miller [ 16/Oct/15 4:39 PM ]

Well, I tried pretty hard but I still can't figure out how to make Android Studio run the project in Genymotion. This was helpful https://www.genymotion.com/#!/developers/user-guide#genymotion-plugin-for-android-studio and I installed the Genymotion plugin etc but I never seem to get the opportunity to choose a device when I run the build.

What I would like to try (in case anyone else is able to do this) is to apply the patch from CLJ-1809 to clojure master, do a build, and then see if that fixes the problem in the Android emulator. If so, then this is just a dupe of CLJ-1809. If not, then probably some more digging is needed.

Comment by Konstantin Mikheev [ 16/Oct/15 4:46 PM ]

Oh no, you don't need the geny plugin for android studio.
It is sad you wasn't able to run it.
I'll run any test builds for you, just let me know when the next version become available.

Comment by Konstantin Mikheev [ 28/Oct/15 3:12 PM ]

I've just tried the org.clojure:clojure:1.8.0-beta2 release and the bug is still there.

Comment by Alex Miller [ 28/Oct/15 4:17 PM ]

I believe you, but I will need more explicit instructions on how to reproduce. (Or someone else needs to track this down.)

Comment by Konstantin Mikheev [ 28/Oct/15 4:25 PM ]

OK, I'll make a series of screenshots.

Comment by Konstantin Mikheev [ 15/Nov/15 3:33 PM ]

Sorry for the delay.

1. At first you need to setup Android Studio: http://developer.android.com/sdk/installing/index.html?pkg=studio

2. And setup the Android SDK: http://developer.android.com/sdk/installing/adding-packages.html

you will need to install:
Tools -> Android SDK Tools,
Tools -> Android SDK Platform Tools,
Tools -> Android SDK Build Tools 23.0.1,
Android 6.0 -> SDK Platform
Extras -> Google Repository (not sure if it is needed)

3. Run Android Studio and open the project.

4. Running the project on the genymotion emulator: https://github.com/konmik/try_clojure_on_android/tree/master/run_with_genymotion

Comment by Alex Miller [ 20/Nov/15 10:03 AM ]

Have you tried 1.8.0-RC2 with this problem?

Comment by Alex Miller [ 20/Nov/15 10:22 AM ]

stop-server uses the locking macro which reminds me of CLJ-1472.

Comment by Konstantin Mikheev [ 20/Nov/15 11:57 AM ]

Yes I've tried, it still doesn't work.

There is something with the new Android bytecode validator.
We had similar problems with validation while using retrolambda.

Comment by Konstantin Mikheev [ 16/Dec/15 3:51 PM ]

Does not work with org.clojure:clojure:1.8.0-RC4 still.

Comment by Alex Miller [ 21/Dec/15 9:29 AM ]

I was able to reproduce this and verify the hypothesis I had above - this is a duplicate of CLJ-1472. The clojure.core/locking macro seems to generate bytecode that fails on the latest ART bytecode validator (that ticket has more detail on this and some links to issues filed on Android in this regard).

Clojure 1.8 is not actually new in this regard - any version of Clojure would fail in the same way as the locking macro has not changed. The difference here is that the new Clojure socket server code in 1.8 causes it to be used during runtime startup, so the failure occurs during bootstrapping when it did not previously.





[CLJ-1827] Reflection warning introduced in CLJ-1259 Created: 13/Oct/15  Updated: 27/Oct/15  Resolved: 27/Oct/15

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: print, reflection, regression
Environment:

1.8.0-beta1


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

 Description   

The patch for CLJ-1259 addressed the reflection warnings in pprint. However, a different ticket introduced some new code in between when this patch was written and applied and thus there is a new reflection warning produced in the Clojure build:

[java] Reflection warning, clojure/pprint/pretty_writer.clj:419:12 - call to method write can't be resolved (target class is unknown).

This ticket is to address that one newly introduced case to remove the warning.

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



 Comments   
Comment by Andy Fingerhut [ 15/Oct/15 10:23 AM ]

Patch clj-1827-v1.patch dated Oct 15 2015 eliminates the one reflection warning in pretty_writer.clj.





[CLJ-1825] Compilation errors on anonymous recursive function Created: 12/Oct/15  Updated: 12/Nov/15  Resolved: 12/Nov/15

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

Type: Defect Priority: Major
Reporter: Nicolas Modrzyk Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

OS X, Yosemite, jdk 1.8.0_60


Approval: Vetted

 Description   

Seems the below does not compile with 1.8:

(def lazy-fib
  "Lazy sequence of fibonacci numbers"
  ((fn rfib [a b]
     (lazy-seq (cons a (rfib b (+' a b)))))
   0 1))

(defn even-lazy-fib[n]
  (filter even? (take n lazy-fib)))

(even-lazy-fib 10)

Status:

  • 1.7.0 - works
  • 1.8.0-alpha2 - works
  • 1.8.0-alpha3-1.8.0-beta1 - VerifyError, see below
  • 1.8.0-beta2 - NPE, see below
  • 1.8.0-RC1 - ClassCastException, see below
  • 1.8.0 master - NPE, see below

1.8.0-alpha3:

CompilerException java.lang.VerifyError: (class: vecperf/bench$rfib__1233, method: invokeStatic signature: (Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;) Illegal local variable number, compiling:(form-init3780016918836504993.clj:3:3)
	clojure.lang.Compiler$InvokeExpr.eval (Compiler.java:3661)
	clojure.lang.Compiler$DefExpr.eval (Compiler.java:455)
	clojure.lang.Compiler.eval (Compiler.java:6948)
	clojure.lang.Compiler.eval (Compiler.java:6906)
	clojure.core/eval (core.clj:3084)
	clojure.core/eval (core.clj:-1)
	clojure.main/repl/read-eval-print--7081/fn--7084 (main.clj:240)
	clojure.main/repl/read-eval-print--7081 (main.clj:240)
	clojure.main/repl/fn--7090 (main.clj:258)
	clojure.main/repl (main.clj:258)
	clojure.main/repl (main.clj:-1)
	clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn--630 (interruptible_eval.clj:58)
Caused by:
VerifyError (class: vecperf/bench$rfib__1233, method: invokeStatic signature: (Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;) Illegal local variable number
	java.lang.Class.getDeclaredConstructors0 (Class.java:-2)
	java.lang.Class.privateGetDeclaredConstructors (Class.java:2658)
	java.lang.Class.getConstructor0 (Class.java:2964)
	java.lang.Class.newInstance (Class.java:403)
	clojure.lang.Compiler$ObjExpr.eval (Compiler.java:4943)
	clojure.lang.Compiler$InvokeExpr.eval (Compiler.java:3652)
	clojure.lang.Compiler$DefExpr.eval (Compiler.java:455)

1.8.0-beta2 / 1.8.0 master:

NullPointerException
	clojure.lang.Numbers.ops (Numbers.java:1013)
	clojure.lang.Numbers.addP (Numbers.java:132)
	user/rfib--1250/fn--1251 (form-init4987495233354047014.clj:4)

1.8.0-RC1:

ClassCastException java.lang.Long cannot be cast to clojure.lang.IFn
	user/rfib--1250/fn--1251 (form-init1118919529313120594.clj:4)


 Comments   
Comment by Alex Miller [ 12/Oct/15 10:07 PM ]

Dupe of CLJ-1809

Comment by Nicolas Modrzyk [ 11/Nov/15 8:51 PM ]

With 1.8-RC1, and the code above, I now get the following:

java.lang.ClassCastException: java.lang.Long cannot be cast to clojure.lang.IFn
/Users/niko/projects/maths/src/maths/fastfib.clj:41 maths.fastfib/rfib[fn]
          LazySeq.java:40 clojure.lang.LazySeq.sval
          LazySeq.java:49 clojure.lang.LazySeq.seq
              RT.java:521 clojure.lang.RT.seq
             core.clj:137 clojure.core/seq
            core.clj:2777 clojure.core/take[fn]
          LazySeq.java:40 clojure.lang.LazySeq.sval
          LazySeq.java:49 clojure.lang.LazySeq.seq
              RT.java:521 clojure.lang.RT.seq
             core.clj:137 clojure.core/seq
            core.clj:2702 clojure.core/filter[fn]
          LazySeq.java:40 clojure.lang.LazySeq.sval
          LazySeq.java:49 clojure.lang.LazySeq.seq
Comment by Alex Miller [ 12/Nov/15 10:26 AM ]

The generated bytecode for rfib seems fishy to me. In 1.7 for example, it does aload_0 to load the this reference to self-invoke, but in 1.8 that winds up in an invokestatic where aload_0 is a, not this, so the stack is messed up when invokespecial is invoked.

1.7:

public java.lang.Object invoke(java.lang.Object, java.lang.Object);
    Code:
       0: new           #14                 // class clojure/lang/LazySeq
       3: dup           
       4: new           #16                 // class fib$rfib__25$fn__26
       7: dup           
       8: aload_0       
       9: aload_1       
      10: aconst_null   
      11: astore_1      
      12: aload_2       
      13: aconst_null   
      14: astore_2      
      15: invokespecial #19                 // Method fib$rfib__25$fn__26."<init>":(Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;)V

In 1.8:

public static java.lang.Object invokeStatic(java.lang.Object, java.lang.Object);
    Code:
       0: new           #14                 // class clojure/lang/LazySeq
       3: dup           
       4: new           #16                 // class fib$rfib__25$fn__26
       7: dup           
       8: aload_0
       9: aload_0
      10: aconst_null
      11: astore_0
      12: aload_1
      13: aconst_null
      14: astore_1
      15: invokespecial #19                 // Method fib$rfib__25$fn__26."<init>":(Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;)V
Comment by Alex Miller [ 12/Nov/15 4:36 PM ]

Rich made a commit to fix this in master:

https://github.com/clojure/clojure/commit/7faeb3a5e1fb183539a8638b72d299a3433fe990

Comment by Nicolas Modrzyk [ 12/Nov/15 6:46 PM ]

Confirming, master with commit "7faeb3a5e1fb183539a8638b72d299a3433fe990" fixes it for me too.





[CLJ-1823] Document new :load-ns option to deftype/defrecord introduced by CLJ-1208 Created: 09/Oct/15  Updated: 27/Oct/15  Resolved: 27/Oct/15

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

Type: Enhancement Priority: Trivial
Reporter: Nicola Mometto Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: defrecord, deftype, docstring

Attachments: Text File 0001-CLJ-1823-document-load-ns-option-to-deftype-defrecor.patch     Text File clj-1823-2.patch    
Patch: Code
Approval: Ok

 Description   

CLJ-1208 was applied in 1.8 alphas and contains a new :load-ns option for defrecord and deftype but did not document that in the docstring.

This ticket adds documentation for that feature to the docstring.

Additionally, text should be added to http://clojure.org/datatypes.

Patch: clj-1823-2.patch



 Comments   
Comment by Alex Miller [ 09/Oct/15 10:54 AM ]

Pulling into 1.8 as it would be nice to doc this in the release.

Comment by Alex Miller [ 12/Oct/15 10:23 AM ]

Modified docstring format slightly, retained attribution in -2 patch.





[CLJ-1812] Recently-added test fails on Windows Created: 06/Sep/15  Updated: 11/Sep/15  Resolved: 11/Sep/15

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: regression
Environment:

Windows


Attachments: Text File CLJ-1812-fix1.patch    
Patch: Code and Test
Approval: Ok

 Description   

Inside of deftest test-pprint-calendar in file test/clojure/test_clojure/pprint/test_pretty.clj, it compares the output of pprint against a string ending in a newline character. This works fine on Linux and Mac OS X, but fails on Windows systems, as there the pprint'ed string ends with a carriage return followed by newline.

Approach: A straightforward fix is to call clojure.string/split-lines on the string, which has a return value that is independent of OS.

Screened: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 06/Sep/15 4:21 PM ]

Patch CLJ-1812-fix1.patch dated Sep 6 2015 allows the test to pass on Windows as well as Mac OS X.

Comment by Alex Miller [ 08/Sep/15 8:02 AM ]

Since this breaks the build on windows, I added it to 1.8 and screened it.





[CLJ-1810] ATransientMap not marked public Created: 31/Aug/15  Updated: 12/Oct/15  Resolved: 12/Oct/15

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

Type: Enhancement Priority: Trivial
Reporter: Gregg Reynolds Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: ft, transient
Environment:

all


Attachments: Text File CLJ-1810-v1.patch    
Patch: Code
Approval: Ok

 Description   

All the other abstract classes are explicitly marked "public". Seems like ATransientMap should be marked like the others.



 Comments   
Comment by Michael Blume [ 31/Aug/15 5:04 PM ]

Here is the obvious patch =)





[CLJ-1809] Compiler produces VerifyError when compiling simple let expression inside a finally block Created: 30/Aug/15  Updated: 27/Oct/15  Resolved: 27/Oct/15

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Fogus
Resolution: Completed Votes: 0
Labels: compiler, regression

Attachments: Text File 0001-CLJ-1809-fix-off-by-one-error-in-direct-linking.patch     Text File clj-1809-2.patch     Text File clj-1809-3.patch    
Patch: Code and Test
Approval: Ok

 Description   

A variant of this issue showed up as it was preventing compilation in ClojureScript.

This is a simplified case (see original in comments):

(defn x [y]
  (try
    (finally
      (let [z y]))))

produces

VerifyError (class: user$x, method: invokeStatic signature: (Ljava/lang/Object;)Ljava/lang/Object;) Can only throw Throwable objects  java.lang.Class.getDeclaredConstructors0 (Class.java:-2)

See below for comparison bytecode.

Cause: In this code, there are locals with these indexes:

0 - this (if not static call)
1 - y (arg)
2 - retLocal (created by compiler to hold result of try body)
3 - finalLocal (created by compiler to hold result of finally body)
4 - z (let local)

The following block was added to FnExpr.parse() for static methods to adjust local binding stack indexes based on not having a "this":

if(fn.canBeDirect){
    for(FnMethod fm : (Collection<FnMethod>)methods)
    {
      if(fm.locals != null)
      {
        for(LocalBinding lb : (Collection<LocalBinding>)RT.keys(fm.locals))
        {
          lb.idx -= 1;
	}
	fm.maxLocal -= 1;
      }
    }
  }

However, in this example locals 2 and 3 are never registered with fn (registerLocal is not called). This (doesn't) happen in TryExpr.parse() where retLocal and finallyLocal call getAndIncLocalNum() but not via registerLocal(). From a search, there are several other places where this happens as well.

The result in the example above is that we end up with the following indexes:

0 - y (arg)
2 - retLocal (created by compiler to hold result of try body)
3 - finalLocal (created by compiler to hold result of finally body)
3 - z (let local)

The overlap in the last 2 indices leads ultimately to the verifier error.

Approach: Make the lb.index adjustment only happen when lb.isArg - these should always be at the beginning of the locals table and therefore reducing their indexes will not affect any other added locals. Also, do not adjust fm.maxlocal (fyi, maxlocal is never used for anything).

Patch: clj-1809-3.patch

Disabling the verifier, here's a dump of the emitted bytecode for inspection

// 1.8
  public static java.lang.Object invokeStatic(java.lang.Object);
    Code:
       0: aconst_null
       1: astore_1
       2: aload_0
       3: aconst_null
       4: astore_0
       5: astore_2
       6: aconst_null
       7: pop
       8: goto 20
      11: astore_2
      12: aload_0
      13: aconst_null
      14: astore_0
      15: astore_2
      16: aconst_null
      17: pop
      18: aload_2
      19: athrow
      20: aload_1
      21: areturn
    Exception table:
       from to target type
           0 2 11 any

Here's the bytecode emitted by clojure 1.7.0 for comparison

// 1.7
  public java.lang.Object invoke(java.lang.Object);
    Code:
       0: aconst_null
       1: astore_2
       2: aload_1
       3: aconst_null
       4: astore_1
       5: astore_3
       6: aconst_null
       7: pop
       8: goto          22
      11: astore        4
      13: aload_1
      14: aconst_null
      15: astore_1
      16: astore_3
      17: aconst_null
      18: pop
      19: aload         4
      21: athrow
      22: aload_2
      23: areturn
    Exception table:
       from    to  target type
           0     2    11   any


 Comments   
Comment by Nicola Mometto [ 30/Aug/15 11:30 PM ]

The attached patch fixes the issue however I'm unfamiliar with the direct linking support in the compiler so I'm not sure this is the right fix.

Comment by Keith Irwin [ 11/Sep/15 12:25 PM ]

I discovered this issue compiling ClojureScript applications using lein-figwheel, which invokes cljsbuild. Using just lein-cljsbuild produces the same issue.

Stack Trace:

Exception in thread "main" java.lang.VerifyError: (class: cljs/util$last_modified, method: invokeStatic signature: (Ljava/lang/Object;)Ljava/lang/Object;) Can only throw Throwable objects, compiling:(util.cljc:142:1)
	at clojure.lang.Compiler$DefExpr.eval(Compiler.java:463)
	at clojure.lang.Compiler.eval(Compiler.java:6939)
	at clojure.lang.Compiler.load(Compiler.java:7381)
	at clojure.lang.RT.loadResourceScript(RT.java:372)
	at clojure.lang.RT.loadResourceScript(RT.java:363)
	at clojure.lang.RT.load(RT.java:453)
	at clojure.lang.RT.load(RT.java:419)
	at clojure.core$load$fn__5661.invoke(core.clj:5883)
	at clojure.core$load.invokeStatic(core.clj:5882)
	at clojure.core$load_one.invokeStatic(core.clj:5683)
	at clojure.core$load_one.invoke(core.clj)
	at clojure.core$load_lib$fn__5610.invoke(core.clj:5728)
	at clojure.core$load_lib.invokeStatic(core.clj:5727)
	at clojure.core$load_lib.doInvoke(core.clj)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invokeStatic(core.clj:647)
	at clojure.core$load_libs.invokeStatic(core.clj:5765)
	at clojure.core$load_libs.doInvoke(core.clj)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invokeStatic(core.clj:647)
	at clojure.core$require.invokeStatic(core.clj:5787)
	at clojure.core$require.doInvoke(core.clj)
	at clojure.lang.RestFn.invoke(RestFn.java:2088)
	at cljs.repl$eval15$loading__5553__auto____16.invoke(repl.cljc:9)
	at cljs.repl$eval15.invokeStatic(repl.cljc:9)
	at cljs.repl$eval15.invoke(repl.cljc)
	at clojure.lang.Compiler.eval(Compiler.java:6934)
	at clojure.lang.Compiler.eval(Compiler.java:6923)
	at clojure.lang.Compiler.load(Compiler.java:7381)
	at clojure.lang.RT.loadResourceScript(RT.java:372)
	at clojure.lang.RT.loadResourceScript(RT.java:363)
	at clojure.lang.RT.load(RT.java:453)
	at clojure.lang.RT.load(RT.java:419)
	at clojure.core$load$fn__5661.invoke(core.clj:5883)
	at clojure.core$load.invokeStatic(core.clj:5882)
	at clojure.core$load_one.invokeStatic(core.clj:5683)
	at clojure.core$load_one.invoke(core.clj)
	at clojure.core$load_lib$fn__5610.invoke(core.clj:5728)
	at clojure.core$load_lib.invokeStatic(core.clj:5727)
	at clojure.core$load_lib.doInvoke(core.clj)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invokeStatic(core.clj:647)
	at clojure.core$load_libs.invokeStatic(core.clj:5765)
	at clojure.core$load_libs.doInvoke(core.clj)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invokeStatic(core.clj:647)
	at clojure.core$require.invokeStatic(core.clj:5787)
	at clojure.core$require.doInvoke(core.clj)
	at clojure.lang.RestFn.invoke(RestFn.java:3204)
	at figwheel_sidecar.repl$eval9$loading__5553__auto____10.invoke(repl.clj:1)
	at figwheel_sidecar.repl$eval9.invokeStatic(repl.clj:1)
	at figwheel_sidecar.repl$eval9.invoke(repl.clj)
	at clojure.lang.Compiler.eval(Compiler.java:6934)
	at clojure.lang.Compiler.eval(Compiler.java:6923)
	at clojure.lang.Compiler.load(Compiler.java:7381)
	at clojure.lang.RT.loadResourceScript(RT.java:372)
	at clojure.lang.RT.loadResourceScript(RT.java:363)
	at clojure.lang.RT.load(RT.java:453)
	at clojure.lang.RT.load(RT.java:419)
	at clojure.core$load$fn__5661.invoke(core.clj:5883)
	at clojure.core$load.invokeStatic(core.clj:5882)
	at clojure.core$load_one.invokeStatic(core.clj:5683)
	at clojure.core$load_one.invoke(core.clj)
	at clojure.core$load_lib$fn__5610.invoke(core.clj:5728)
	at clojure.core$load_lib.invokeStatic(core.clj:5727)
	at clojure.core$load_lib.doInvoke(core.clj)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invokeStatic(core.clj:647)
	at clojure.core$load_libs.invokeStatic(core.clj:5765)
	at clojure.core$load_libs.doInvoke(core.clj)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invokeStatic(core.clj:647)
	at clojure.core$require.invokeStatic(core.clj:5787)
	at clojure.core$require.doInvoke(core.clj)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at user$eval5.invokeStatic(form-init6950918879748180251.clj:1)
	at user$eval5.invoke(form-init6950918879748180251.clj)
	at clojure.lang.Compiler.eval(Compiler.java:6934)
	at clojure.lang.Compiler.eval(Compiler.java:6923)
	at clojure.lang.Compiler.load(Compiler.java:7381)
	at clojure.lang.Compiler.loadFile(Compiler.java:7319)
	at clojure.main$load_script.invokeStatic(main.clj:275)
	at clojure.main$init_opt.invokeStatic(main.clj:277)
	at clojure.main$init_opt.invoke(main.clj)
	at clojure.main$initialize.invokeStatic(main.clj:308)
	at clojure.main$null_opt.invokeStatic(main.clj:342)
	at clojure.main$null_opt.invoke(main.clj)
	at clojure.main$main.invokeStatic(main.clj:421)
	at clojure.main$main.doInvoke(main.clj)
	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.VerifyError: (class: cljs/util$last_modified, method: invokeStatic signature: (Ljava/lang/Object;)Ljava/lang/Object;) Can only throw Throwable objects
	at java.lang.Class.getDeclaredConstructors0(Native Method)
	at java.lang.Class.privateGetDeclaredConstructors(Class.java:2671)
	at java.lang.Class.getConstructor0(Class.java:3075)
	at java.lang.Class.newInstance(Class.java:412)
	at clojure.lang.Compiler$ObjExpr.eval(Compiler.java:4923)
	at clojure.lang.Compiler$DefExpr.eval(Compiler.java:450)
	... 93 more
Comment by Keith Irwin [ 11/Sep/15 12:39 PM ]

Reproducible using ClojureScript 1.7.122, but not 1.7.48 or 1.7.58.

Comment by Keith Irwin [ 11/Sep/15 12:43 PM ]

Here's where the error is thrown:

https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/util.cljc#L142-L155

(defn last-modified [src]
  (cond
    (file? src) (.lastModified ^File src)
    (url? src)
    (let [conn (.openConnection ^URL src)]
      (try
        (.getLastModified conn)
        (finally
          (let [ins (.getInputStream conn)]
            (when ins
              (.close ins))))))
    :else
    (throw
      (IllegalArgumentException. (str "Cannot get last modified for " src)))))
Comment by Nicola Mometto [ 11/Sep/15 12:50 PM ]

Keith Irwin yeah that's how the bug originally got reported and that's the function I used to find a minimal reproducible example

Comment by Alex Miller [ 18/Sep/15 10:41 AM ]

clj-1809-2.patch is identical to prior patch, just updated to apply to current master.

Comment by Fogus [ 09/Oct/15 12:35 PM ]

With some digging I was able to determine the problem and how the solution works to fix that problem. In the future, whenever reporting bytecode verification errors it might help to show the equivalent Java code pertaining to the problemmatic bytecode.

Comment by Rich Hickey [ 26/Oct/15 3:00 PM ]

Thanks for chasing this down Nicola.





[CLJ-1805] :rettag encourages wrong runtime type hints Created: 28/Aug/15  Updated: 11/Nov/15  Resolved: 09/Nov/15

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: typehints

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

 Description   

Currently :rettag works only for expressions like:

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

or

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

But at runtime those type-hints are resolved to

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

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

Patch: clj-1805.patch fixes the bad boolean logic mentioned in the comments.



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

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

Comment by Andy Fingerhut [ 11/Sep/15 3:58 PM ]

Alex, it appears that the code commented on by David Miller definitely has a bug, with a simple fix (change || to && would be one fix, I believe). Should there be a separate ticket for that fix from this ticket?

Comment by Alex Miller [ 29/Sep/15 11:53 AM ]

It seems like the existing code also works for things like:

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

?

Comment by Nicola Mometto [ 29/Sep/15 12:09 PM ]

Yes, which is equally useless:

user=> (set! *warn-on-reflection* true)
true
user=> (defn ^"long" a [] 1)
#'user/a
user=> (loop [x 0] (if (= x 1) x (recur (a))))
NO_SOURCE_FILE:3 recur arg for primitive local: x is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: x
1
Comment by Nicola Mometto [ 28/Oct/15 6:01 AM ]

I'm reopening this issue as the committed patch doesn't deal with the issues I've originally opened this ticket for.
Namely that I think that in its current incarnation there is no value in :rettag as all its possible usages end up in an invalid tag either at runtime or at compile time.
Additionally it breaks the eastwood linter, an issue that I would be willing to fix on the eastwood side if the feature was valuable but I don't think this is the case.

Comment by Alex Miller [ 28/Oct/15 9:03 AM ]

Can you explain how this breaks Eastwood?

Comment by Andy Fingerhut [ 28/Oct/15 9:49 AM ]

I haven't looked into it in enough detail to fix it yet, but I am pretty sure that the reason Eastwood misbehaves with Clojure 1.8.0, at least since :rettag was added, is simply because Eastwood assumes that some ASTs will be the direct children of other ASTs, but when :rettag was added, they now have a new intermediate AST node between them. I haven't modified Eastwood to handle that change yet, but as Nicola mentioned, it should be straightforward to generalize Eastwood's AST handling here.

Comment by Alex Miller [ 28/Oct/15 10:00 AM ]

That sounds different than the problem described in the subject of this ticket.

Comment by Andy Fingerhut [ 28/Oct/15 10:04 AM ]

I'm not sure I'm understanding Nicola here, but wanted to ask a question that may clarify it.

Nicola, are you saying that with :rettag, it doesn't actually make things worse for adding tags in Clojure source code, but it isn't clear to you that it makes anything better, either, so why bother making such a change?

That is, in 1.8.0-beta2, functions like (defn ^long x [...] ...) still have a type tag that is evaluated to the function called 'long', and are thus incorrect type tags as they were in Clojure 1.7.0, so what use is :rettag in improving anything?

Comment by Nicola Mometto [ 28/Oct/15 12:39 PM ]

Right, what I was trying to say is that there is no apparent bug caused by the current implementation of :rettag, however:

  • it doesn't seem to be useful at all given that all the possible usages seem lead to an invalid tag
  • it complicates the compiler for no apparent reason
  • it breaks eastwood and possibly other libraries that rely on the current concrete expansion of defn

I would not bring the last point up if the feature was in any way valuable, but given that it doesn't seem so, I brought it up to make the point about why I'd like this change to be reverted or revisited.

Comment by Andy Fingerhut [ 29/Oct/15 9:32 AM ]

As far as I know (I haven't dug into the :rettag implementation the way Nicola has), (defn ^long x [...] ...) was a useless type tag in Clojure 1.7 and earlier, and it is still a useless type tag in Clojure 1.8-beta2.

(defn {:tag 'long} x [...] ...) is a useful type tag in Clojure 1.7 and earlier (back to some version number I'm not going to check right now), and it is still a useful type tag in Clojure 1.8-beta2.

Is that all correct?

Are there any type tags whose behavior changes from Clojure 1.7 to 1.8-beta2?

Comment by Nicola Mometto [ 29/Oct/15 10:05 AM ]

Correct.
:rettag was never intended (as far as I understan by RIch's commits) to have any behavioural change, rather it should have served as an optimization to avoid checkcasts/boxing w/ direct-linking, but it doesn't work so I see no point in keeping non-working code around that might even cause some tooling libraries to error-out (like eastwood).

Comment by Andy Fingerhut [ 30/Oct/15 1:58 PM ]

Nicola, could it be that Rich wants to add :rettag to make a single common place to go to find that information that is in metadata on the function, as opposed to more deeply hidden inside the compiler?

Comment by Nicola Mometto [ 31/Oct/15 10:00 AM ]

No, :rettag is still just compile-time metadata

Comment by Alex Miller [ 09/Nov/15 3:48 PM ]

I'm re-closing this as the ticket really was intended to be a question about rettag's role and purpose. I have added that to a list of items to discuss with Rich.

Comment by Nicola Mometto [ 11/Nov/15 1:18 PM ]

I'm hoping to have the questions I've raised re: why are we keeping :rettag around if it serves no purpose answered before 1.8 is released





[CLJ-1785] Reader conditionals throws when they have nil expressions Created: 21/Jul/15  Updated: 31/Jul/15  Resolved: 31/Jul/15

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

Type: Defect Priority: Critical
Reporter: Jozef Wagner Assignee: Jozef Wagner
Resolution: Completed Votes: 1
Labels: reader, readerconditionals

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

 Description   

Reader conditional that has nil as an expression fails.

e.g. (read-string {:read-cond :allow} "#?(:default nil)")

The fact that nil values are valid expressions are documented at both official documentation and design page.

Patch: clj-1785-v2.patch

Screened by: Alex Miller



 Comments   
Comment by Jozef Wagner [ 21/Jul/15 3:53 PM ]

Added patch with tests

Comment by Jozef Wagner [ 21/Jul/15 4:06 PM ]

v2 patch that uses static final sentinel value

Comment by Nicola Mometto [ 22/Jul/15 7:23 AM ]

CLJ-1138 reports a similar bug with data readers.





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

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

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

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

 Description   

Seen in a tweet...

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

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

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

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

Screened by: Alex Miller



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

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

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

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

Updated patch with two changes:

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

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

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

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

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

Is this the smallest change that can be made?

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

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

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

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

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

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

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

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

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

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

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

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

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

The current patch makes the following changes:

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

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

Please excuse the rather lengthy reply.

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

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

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





[CLJ-1772] Spelling mistake in clojure.test/use-fixtures Created: 01/Jul/15  Updated: 31/Jul/15  Resolved: 31/Jul/15

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

Type: Defect Priority: Trivial
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft

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

 Description   

Part of the docstring for `use-fixtures` is:

individually, while:once wraps the whole run in a single function.

this should be

individually, while :once wraps the whole run in a single function.

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 01/Jul/15 12:42 AM ]

If you can get me a patch, happy to pre-screen for next release.

Comment by Daniel Compton [ 01/Jul/15 12:43 AM ]

2015-07-01 17:43:02





[CLJ-1769] Docstrings for *' and +' refer to * and + instead of *' and +' Created: 28/Jun/15  Updated: 31/Jul/15  Resolved: 31/Jul/15

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

Type: Defect Priority: Trivial
Reporter: Mark Simpson Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft

Attachments: Text File docstringfix.patch    
Patch: Code
Approval: Ok

 Description   

The docstrings for *' and +' refer to the behavior of * and + if they are passed no arguments. The docstrings should refer to the behavior of *' and +' instead.



 Comments   
Comment by Andy Fingerhut [ 02/Jul/15 3:49 PM ]

Mark, your patch "patch.txt" is not in the expected format for a patch, and please use a file name ending with ".diff" or ".patch", for the convenience of patch reviewers. See this link for instructions on creating a patch in the expected format: http://dev.clojure.org/display/community/Developing+Patches

Comment by Mark Simpson [ 02/Jul/15 4:36 PM ]

Sorry about that. Hopefully I got things right this time.





[CLJ-1766] Serializing+deserializing lists breaks their hash Created: 21/Jun/15  Updated: 03/Sep/15  Resolved: 03/Sep/15

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

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

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


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

 Description   

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

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

Examples:

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

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

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

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

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

Alternate approaches:

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

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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





[CLJ-1765] areduce speed improvements Created: 19/Jun/15  Updated: 12/Oct/15  Resolved: 12/Oct/15

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

Type: Enhancement Priority: Major
Reporter: Karsten Schmidt Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: ft, performance
Environment:

OSX 10.8.5, Oracle JDK1.8.0_25-b17


Attachments: Text File clj-1765-2.patch     Text File clj-1765.patch    
Patch: Code
Approval: Ok

 Description   

Two performance improvements for areduce:

1. Call alength once, rather than every iteration
2. Use unchecked-inc-int instead of inc since array indices are limited to int range

Example:

(def a (long-array (range 1000)))
(areduce a i ret 0 (+ ret (aget a i)))

Criterium quick-bench:

  • 1.7.0-RC2: 15.5 ms
  • RC2+patch: 7.7 ms

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



 Comments   
Comment by Karsten Schmidt [ 19/Jun/15 7:08 PM ]

added patch w/ changes described above

Comment by Alex Miller [ 09/Oct/15 8:27 AM ]

Updated -2 patch to put ticket id at beginning of commit message and to widen context lines. No semantic changes, attribution retained.





[CLJ-1761] clojure.core/run! does not always return nil Created: 17/Jun/15  Updated: 17/Jul/15  Resolved: 17/Jul/15

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

Type: Defect Priority: Major
Reporter: Jonas Enlund Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File clj-1761.patch     Text File clj-1761-with-tests.patch    
Patch: Code and Test
Approval: Ok

 Description   

According to the documentation clojure.core/run! should return nil. This is not the case as seen by the following examples:

user=> (run! identity [1])
1
user=> (run! reduced (range))
0

Approach: return 'nil'

Patch: clj-1761-with-tests.patch

Screened by: Alex Miller






[CLJ-1745] Some compiler exceptions wrapped in new CompilerException Created: 04/Jun/15  Updated: 17/Jun/15  Resolved: 17/Jun/15

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: compiler, error-reporting, regression

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

 Description   

Clojure error reporting changed in CLJ-1169 to wrap exceptions thrown during macro evaluation in CompilerException to give more input:

Clojure 1.6

(defmacro demo [] (throw (ex-info "fail" {})))
(demo)
ExceptionInfo fail  clojure.core/ex-info (core.clj:4403)
(class *e)
=> clojure.lang.ExceptionInfo

Clojure 1.7.0-alpha2 to 1.7.0-RC1

(defmacro demo [] (throw (ex-info "fail" {})))
(demo)
ExceptionInfo fail  clojure.core/ex-info (core.clj:4403)
;; NOTE: lein repl will instead print: CompilerException clojure.lang.ExceptionInfo: fail {}, compiling:(form-init8304622754337237403.clj:1:1)
(class *e)
=> clojure.lang.Compiler$CompilerException

This change has caused some breakage for users that throw exceptions in macros and expect to see the same exception at the top of the exception chain, not wrapped in a CompilerException. This change is somewhat masked in the Clojure REPL because clojure.main/root-cause unwraps CompilerException wrappers and prints the root cause when an exception occurs.

More background can be found in some messages on:
https://groups.google.com/d/msg/clojure/ccZuKTYKDPc/xpaz44UDqYwJ

Approach: The attached patch rolls back most of the change for CLJ-1169, specifically the part that wraps exceptions in CompilerException and the tests that were affected by this change (good examples of the kind of breakage others are seeing). I left the parts of CLJ-1169 that added quotes in the error message and those equivalent tests.

Patch: clj-1745.patch



 Comments   
Comment by Stuart Halloway [ 04/Jun/15 7:15 PM ]

All the stuff about lein in this ticket is just noise, and I am removing it. (Please don't use the phrase "small reproducing case" for anything that includes lein.) Clojure's behavior changed: improved error reporting. Code that explicitly relied on less-good error reporting broke.

Comment by Alex Miller [ 05/Jun/15 7:50 AM ]

Pulling this into 1.7 just for tracking discussion.

Comment by Alex Miller [ 05/Jun/15 4:02 PM ]

There is one confusing factor in replicating this re lein vs Clojure. The Clojure REPL will unpeel CompilerExceptions (see clojure.main/root-cause) so the repl actually prints the same in 1.7 as in 1.6 (but the exception chain and class are wrapped in one more CompilerException than before). Leiningen's repl will actually show the full structure.

Comment by Alex Miller [ 05/Jun/15 4:36 PM ]

I went back and looked at CLJ-1169 and while I think the intentions are good there, I do think that wrapping exceptions that happen to be thrown out of macro bodies like this does create unexpected (certainly different) behavior. I've attached a patch that rolls back most of the CLJ-1169 change.





[CLJ-1738] Document that seqs are incompatible with Java iterators that return the same mutable object every time Created: 27/May/15  Updated: 18/Jul/15  Resolved: 17/Jun/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: Stuart Halloway
Resolution: Completed Votes: 0
Labels: regression
Environment:

1.7.0-RC1


Attachments: Text File clj-1738-2.patch     Text File clj-1738-3.patch     Text File clj-1738-4.patch     Text File clj-1738-doc.patch     Text File clj-1738.patch    
Patch: Code
Approval: Ok

 Description   

Some Java libraries return iterators that return the same mutable object on every call:

  • Hadoop ReduceContextImpl$ValueIterator
  • Mahout DenseVector$AllIterator/NonDefaultIterator
  • LensKit FastIterators

While careful usage of seq or iterator-seq over these iterators worked in the past, that is no longer true as of the changes in CLJ-1669 - iterator-seq now produces a chunked sequence. Because next() is called 32 times on the iterator before the first value can be retrieved from the seq, and the same mutable object is returned every time, code on iterators like this now receives different (incorrect) results.

Approach: Sequences cache values and are thus incompatible with holding mutable and mutating Java objects. We will add some clarification about this to seq and iterator-seq docstrings. For those iterators above, it is recommended to either process those iterators in a loop/recur or to wrap them in a lazy-seq that transforms each re-returned mutable object into a proper value prior to caching.

Patch: clj-1738-doc.patch



 Comments   
Comment by Alex Miller [ 27/May/15 7:00 AM ]

I spot-checked some of the perf timings from CLJ-1669 and didn't see anything unexpected.

Comment by Marshall T. Vandegrift [ 27/May/15 7:38 AM ]

In order to maintain compatibility it is also necessary to change `clojure.lang.RT/seqFrom` back to creating non-chunked `IteratorSeq`s. I've verified that these changes are sufficient to restore compatibility for my cases.

Comment by Marshall T. Vandegrift [ 27/May/15 10:05 AM ]

Added updated version of proposed patch which covers RT Iterable->seq coercion and includes a test case.

Comment by Alex Miller [ 01/Jun/15 6:39 AM ]

The seqFrom change is good but I'd prefer not to add the Java class in the test. Can you replace that with a deftype implementing Iterable to reify an Iterator?

Comment by Marshall T. Vandegrift [ 01/Jun/15 10:32 AM ]

Added updated version of patch with pure-Clojure implementation of mutation-based iterator test.

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

I re-ran the full perf tests from CLJ-1669 and did not see any real changes except for in the last sort over eduction ones. We should still be seeing chunked iterator sequences over eductions which was the primary intent of the original change. We've just fallen back to non-chunked as we had before in the general case.

Comment by Alex Miller [ 05/Jun/15 2:39 PM ]

I think this looks good but since I had a hand in the early development of the patch I'm going to suggest that Stu screen it.

Comment by Alex Miller [ 09/Jun/15 1:18 PM ]

Marshall, can you update the patch so eduction's docstring says "reducible/iterable/seqable" and "reduce/iterator/seq"?

Comment by Marshall T. Vandegrift [ 09/Jun/15 3:08 PM ]

No problem. Updated and attached, but I've also changed the patch author to myself and fleshed out the commit message – if I'm going to do the drudge work I might as well take the credit too!

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

No problem at all - thanks!

Comment by Alex Miller [ 17/Jun/15 9:33 AM ]

Direction of this ticket changed at Rich's request.

Prior description capture here:

Clojure code that uses iterator-seq to wrap Java iterators that return the same mutable object on every call are broken by the chunked iterator-seq changes from CLJ-1669.

Some examples where this occurs:

  • Hadoop ReduceContextImpl$ValueIterator
  • Mahout DenseVector$AllIterator/NonDefaultIterator
  • LensKit FastIterators

Cause: In 1.6, the iterator-seq wrapper could be used with these to consume a sequence over these iterators element-by-element. In 1.7 RC1, iterator-seq produces a chunked sequence. Because next() is called 32 times on the iterator before the first value can be retrieved from the seq, and the same mutable object is returned every time, code doing this now receives different (incorrect) results.

Approach: Switch iterator-seq back to non-chunked and change eduction to use the chunking iterator-seq strategy as that was the original target. Retain the use of the chunked iterator seq in sequence over the TransformerIterator.

Patch: clj-1738-4.patch

Comment by Marshall T. Vandegrift [ 17/Jun/15 9:57 AM ]

Sorry, what just happened here? Is this no longer being fixed?

Comment by Alex Miller [ 17/Jun/15 10:06 AM ]

Hey Marshall, I thought you might have some questions. As noted above, Rich decided that this should not be a valid usage of seqs over these kinds of iterators (even though your usage happened to suffice in the past). So, you should alter your code to use these iterators in a different way.

Comment by Marshall T. Vandegrift [ 17/Jun/15 10:08 AM ]

Will there be a "breaking changes" section in the release notes for 1.7?

Comment by Alex Miller [ 17/Jun/15 11:20 AM ]

I will add a compatibility section. In this case, it should be considered "already broken" (but you're just now aware of it) I think.

Comment by Mike Rodriguez [ 17/Jun/15 10:09 PM ]

The main question I have is what is the proposed alternative way to interact with these object reusing iterators? I struggle to see what Clojure functions are safe to use on them because anything that internally calls seq must be avoided.

Comment by Alex Miller [ 18/Jun/15 5:23 AM ]

I would say that you shouldn't expect any sequence functions (all of which coerce to seq) to give you useful results. Instead, either consume the iterator in a loop/recur or create a lazy-seq that transforms each re-returned mutable object into a proper value prior to caching.

Comment by Marshall T. Vandegrift [ 18/Jun/15 7:11 AM ]

I expect at this point it isn't possible to change in Clojure/core's mind, but, Alex, your last comment crystallized my specific objection to this change.

You suggest "create a lazy-seq that transforms each re-returned mutable object into a proper value prior to caching." When the seq element-realization semantics match the element-at-a-time `Iterator` element-realization semantics there's an exact function for this: `map`. Specifically that this change breaks existing, working instances of the pattern `(map get-value iterable)` to me clearly demonstrates that the change is not compatible with the semantics of the `Iterator` interface. The fact that the newly-brokenness of the pattern is so non-obvious just emphasizes the point.

An unknown amount of deployed code in the Clojure ecosystem, and a non-trivial amount in my own code bases, are currently using this pattern to handle mutating-element Iterators. From my own tally of used Java-ecosystem libraries which include this pattern, I believe the mutating-Iterator case tmore common than Clojure/core apparently expect. For myself and all the other developers using Clojure to orchestrate large and obtuse Java frameworks, I plea for compatibility.

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

"When the seq element-realization semantics match the element-at-a-time `Iterator` element-realization semantics" makes an incorrect assumption. As a general guideline, code that relies on how many or when elements of a lazy seq are realized is fragile - Clojure does not make guarantees about that.

Additionally, there is another assumption that the iterator seq will only be traversed once, as you will get different and incorrect results after the first time.

Use of a seq built on this kind of iterator violated these assumptions, even if it happened to work.

Comment by Marshall T. Vandegrift [ 18/Jun/15 8:47 AM ]

I respectfully disagree.

"As a general guideline, code that relies on how many or when elements of a lazy seq are realized is fragile - Clojure does not make guarantees about that." This is true about seqs in general, but the Iterator interface does guarantee that a single element at a time is realized. I strong believe that a correct interop abstraction for generating seqs from Iterators must maintain this guarantee. I'm not making a claim about seqs in general, just for Iterator->seq coercion in order to maintain the semantics of the underlying Iterator and thus provide a useful & correct interop facility.

"Additionally, there is another assumption that the iterator seq will only be traversed once, as you will get different and incorrect results after the first time." Either seqs cache or they don't, yes? I don't believe it is coherent to argue both that mutation is incompatible due to caching and that `map`ing is incompatible because there might not be caching.

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

The issue is with iterators that return elements that aren't values. Seqs cache values. If you're not using values as elements, then you are outside the bounds of what is supported.

Comment by Marshall T. Vandegrift [ 18/Jun/15 9:40 AM ]

I agree that's their primary intent, but then why functions like `dorun`? For most of Clojure's history seqs have been the primary abstraction for composable iteration over linear collections. With Clojure 1.7 in particular introducing a variety of finer-grained abstractions, I agree this more sharply defines the primary/optimal use for seqs. But this shouldn't come at the cost of invalidating existing code which uses only public interfaces or introducing mismatches with fundamental host platform abstractions like Iterator.

Comment by Mike Rodriguez [ 18/Jun/15 10:28 AM ]

"This is true about seqs in general, but the Iterator interface does guarantee that a single element at a time is realized."

-
This part makes it look like seqs and the Iterator interface are not compatible with one another and Clojure is just pretending they can be.

-
Having a chunking behavior paired with Java Iterators is going to be unreliable because the caller hasn't had a chance to see intermediate elements as they were consumed from the Iterator.

I'm still having difficulty in trying to understand how to interop with an API. My particular case is the (very popular) Hadoop ReduceContextImpl$ValueIterator. I tend to agree with Clojure's strong stance on values and against mutable state like this iterator uses. However, Hadoop apparently has done this from a practical standpoint where the cost of a very large number of object allocations outweighed the cost of adding the mutable state complexity. In this case, Hadoop still did uphold the contract for an Iterator and it made sense for consumers to deal with it against that contract.
When this enters Clojure, we may wrongfully interact with this Iterator as a chunking seq, when it really is not going to be match.

I've been using Clojure for a few years now in my full-time work and this scares me only because I struggle to know what functions I call that may inadvertently "chunk" the Iterator I'm interop'ing with from Java. If I had more clarity on that issue, I may be more comfortable with this. I still don't think the Iterator iterface should really be treated as "chunkable" with by seq though.

Comment by Alex Miller [ 18/Jun/15 11:08 AM ]

There are two ways to interop with an iterator like this - consume it in a loop/recur, or wrap it in a lazy seq. The latter is more similar to whatever you were already doing, so you'd want something like:

(defn iter-seq [iter f]
  (if (.hasNext iter)
    (lazy-seq
      (cons (f (.next iter))
            (iter-seq iter f)))))

which applies a function f to convert from the mutable thing returned from iter to a value. Apply this before doing anything else, then use the result as a normal seq.

Example using the mutating-iterable in the clj-1738-4.patch and AtomicLong.get() as f:

(let [mi (mutating-iterable 10)
      iter (.iterator mi)
      s (iter-seq iter #(.get %))]
    (println s)    ;; (0 1 2 3 4 5 6 7 8 9)
    (println s))   ;; (0 1 2 3 4 5 6 7 8 9)

Again, the real problem here is having a seq that contains mutating objects instead of values. Chunking just exposes that as the problem. If you care about whether chunking happens, then something is wrong.

Comment by Mike Rodriguez [ 18/Jun/15 12:11 PM ]

Thanks Alex. I appreciate the feedback. I certainly think this is valuable and a technique that I will keep in mind to avoid these issues.
My current real-world issue is in my usage of the Hadoop Iterable that has the mutating Iterator behind it. I have currently be using a `reduce` over this Iterable.
In this case, I believe I am safe since `reduce` operates at a different abstraction than the seq abstraction. I believe that is still a correct way to deal with this, but let me know if I'm mistaken.

For completeness, I'd like to make one more point on this topic in regards to "If you care about whether chunking happens, then something is wrong" with respect to Iterators.

I do not think mutability is the only concern of the element-at-a-time contract of an Iterator. The Iterator interface can be used as a stream of elements. This stream allows the consumer to view an individual element at a time, and decide what to do from there - copy it, pull some (derived) value from it, etc.
e.g. The consumer may decide to just look at an individual field of that element and then not need the element at all anymore. A common case is to iterate over an Iterator of items to calculate some summary value. Perhaps the elements are very memory intensive and we do intend to try and fit multiple (to some n count of elements) into memory all at the same time.

My key point is that the Iterator interface leaves the decision of whether or not to hold references to the elements on successive next() calls to the consumer.
Clojure's seq on Iterators makes a decision for the consumer that they can handle having a chunk (e.g. 32 elements) consumed from the Iterator at once. This doesn't have to strictly be a problem of mutable state. This can be about other resource management issues - such as memory in my above example.

I could also see it being that the Iterator is providing elements to the consumer that hold open some resources while the element is being "looked at". When hasNext() is called the Iterator impl could decide to close old connections to resources used in past iterations.
The consumer does not get to have a chance to look at some of these elements at the time they are available anymore, due to the assumption that Clojure makes of being able to read from the Iterator in chunks prior to the consumer seeing the items.

Again, I think I agree that caching and chunking of seqs is at odds with the contract of Iterator. It is because of this, that I find it sneaky how Clojure may behave with them in these circumstances.
I suppose that a seq for Iterator is really only for a special class of Iterators, where there is no concern for holding a chunk in memory at a time or the resource usage to realize a chunk at a time when only a single element may be needed at that point. This is the reality of how laziness interacts with chunking already though for the most part - things will may be lazy, but not necessarily one element at a time lazy.

I certainly can see this change of behavior sneaking up and breaking libraries out there that are interoping with Java Iterators at this point though.

Comment by Marshall T. Vandegrift [ 18/Jun/15 12:14 PM ]

I do understand the available solutions. My dual concern is that they should not be necessary, and that it will not be immediately clear in existing code where the solutions need to be applied.

It seems that I'm not going to be able to convince you, and have no ability to even attempt to convince Rich etc. I'll probably go the route of using a internally-patched version where I want to upgrade existing applications to Clojure 1.7.

Even though we could not come to agreement, I appreciate the time you've taken discussing this issue and seeking resolution for it. Thank you!

Comment by Alex Miller [ 18/Jun/15 2:11 PM ]

While I think you can see the interface of iterators and seqs in a similar way, I perceive them very differently.

I see Java iterators as a stateful interface for external iteration of a source - that is, they provide a processing model. Being stateful, iterators are (in most cases) not safe to share across threads. Once you create one, you have to control access to it.

I see seqs as being a logical list data structure that may have a variety of strategies for production. Caching and immutability are very much bound up in that sense that seqs can be treated as data, passed around safely, etc even if they are built initially on demand.

The sequence you are producing from one of these iterators will give you different results if looked at more than once. This feels deeply wrong to me from a Clojure perspective - as a seq user this violates every conception I have. Yes, you could (previously) use that seq as a form of iteration, but imo that's an abuse of knowing too much about the implementation. If you care about allocation costs, then using a seq that creates and caches seq nodes is a waste of memory. If you care about resource management, laziness is also a bad fit for that need as it's difficult to know when a resource has been completed.

Instead of trying to wedge everything into sequences, consider your new options in 1.7! You could use an eduction to delay processing but eagerly process a stack of transformations without allocation on an iterable source when it's time to do so. Or transduce/into/etc to do it eagerly. Or even sequence to compute it incrementally, which is actually a better answer than the lazy-seq one I gave above. reduce does walk the iterator one-by-one (there's no other way to do it!), and will apply the reducing function to each element before obtaining the next, so using either sequence (if you want caching) or eduction (if you just want delay) or into or reduce/transduce all in combination with a map transducer that produces a value, is another good solution in 1.7:

user=> (def to-val #(.get %)) ;; mutable object to value 
#'user/to-val 
user=> (into [] (map to-val) (mutating-iterable 10)) 
[0 1 2 3 4 5 6 7 8 9] 
user=> (eduction (map to-val) (mutating-iterable 10)) 
(0 1 2 3 4 5 6 7 8 9) 
user=> (sequence (map to-val) (mutating-iterable 10)) 
(0 1 2 3 4 5 6 7 8 9)

All of those are just taking a single "value-convert" but could instead take an arbitrary composition of transducers instead (which will incur none of the intermediate seq node allocation vs using lazy seqs). So thanks for mentioning reduce Mike - those neurons hadn't connected in my head yet.

Comment by Mike Rodriguez [ 19/Jun/15 11:29 AM ]

After reading through your last response I can say I feel more comfortable about this change and the appropriate way to deal with these types of Iterators now.

I appreciate you going into all that detail to explain this. It looks like 1.7 has a lot to offer in allowing for these more "fine-grained" ways to interact with collections like this. +1 to that!

Comment by Ghadi Shayban [ 18/Jul/15 2:44 AM ]

Field report of this breaking: https://github.com/aphyr/tesser/blob/82f2c36915b036137b0e4d97aacebfa793db6b98/math/src/tesser/quantiles.clj#L101-L105





[CLJ-1736] Tweaks to changelog for 1.7 RC2 Created: 22/May/15  Updated: 17/Jun/15  Resolved: 17/Jun/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: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

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

 Description   

Just some minor tweaks to the changelog.



 Comments   
Comment by Nicola Mometto [ 22/May/15 11:37 AM ]

https://github.com/clojure/clojure/commit/69afe91ae07a4c75c34615a4af14327f98d78510#commitcomment-10670998





[CLJ-1735] Throwable->map is missing docstring and since Created: 22/May/15  Updated: 17/Jun/15  Resolved: 17/Jun/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: docstring

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

 Description   

Throwable->map is missing docstring and since






[CLJ-1728] source fn fails for fns with conditional code Created: 10/May/15  Updated: 05/Jun/15  Resolved: 12/May/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: Mike Fikes Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: reader, repl
Environment:

1.7.0-beta2


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

 Description   

Note: Similar to issue CLJS-1261.

If you use the source Clojure REPL function on a function defined in a CLJC file, where the function itself contains some conditional code, then the operation will fail with "Conditional read not allowed".

To reproduce:
Do a lein new testme, rename the core.clj file to core.cljc, and then add the following

(defn f 
  "Eff"
  [] 
  1)

(defn g 
  "Gee"
  []
  #?(:clj "clj" :cljs "cljs"))

Additionally, revise the project.clj to specify 1.7.0-beta2.

Require the testme.core namespace, referring :all.

Verify that you can call, get the doc for, and source for f.

But, on the other hand, while you can call and get the doc for g, you can't do (source testme.core/g).

user=> (source testme.core/g)

RuntimeException Conditional read not allowed  clojure.lang.Util.runtimeException (Util.java:221)
user=> (pst)
RuntimeException Conditional read not allowed
	clojure.lang.Util.runtimeException (Util.java:221)
	clojure.lang.LispReader$ConditionalReader.checkConditionalAllowed (LispReader.java:1406)
	clojure.lang.LispReader$ConditionalReader.invoke (LispReader.java:1410)
	clojure.lang.LispReader$DispatchReader.invoke (LispReader.java:682)
	clojure.lang.LispReader.read (LispReader.java:255)
	clojure.lang.LispReader.readDelimitedList (LispReader.java:1189)
	clojure.lang.LispReader$ListReader.invoke (LispReader.java:1038)
	clojure.lang.LispReader.read (LispReader.java:255)
	clojure.lang.LispReader.read (LispReader.java:195)
	clojure.lang.LispReader.read (LispReader.java:190)
	clojure.core/read (core.clj:3638)
	clojure.core/read (core.clj:3636)
nil

Approach: Set {:read-cond :allow} if source file extension is .cljc. Test above works now.

Patch: clj-1728.patch



 Comments   
Comment by Mike Fikes [ 11/May/15 8:05 AM ]

I tested with Alex's cli-1728.patch, and it works for me.

Comment by Mike Fikes [ 12/May/15 7:02 PM ]

Confirmed fixed using master.

Comment by Nicola Mometto [ 05/Jun/15 9:53 AM ]

Patch committed and the ticket marked as resolved but not closed. I'm closing it.





[CLJ-1727] range confused by large bounds Created: 07/May/15  Updated: 05/Jun/15  Resolved: 12/May/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: Steve Miner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: regression

Attachments: Text File clj-1709-wip-1.patch     Text File clj-1709-wip-2.patch     Text File clj-1727-2.patch     Text File clj-1727-3.patch     Text File clj-1727-4.patch     Text File clj-1727.patch    
Patch: Code and Test
Approval: Ok

 Description   

There are a number of issues related to counting and overflow in the current LongRange implementation.

expression 1.6.0 1.7.0-beta2 +patch comment
(range (- Long/MAX_VALUE 2) Long/MAX_VALUE) (9223372036854775805 9223372036854775806) OOME (9223372036854775805 9223372036854775806) top of long range
(count (range (- Long/MAX_VALUE 2) Long/MAX_VALUE)) 2 2 2 top of long range
(range (+ Long/MIN_VALUE 2) Long/MIN_VALUE -1) (-9223372036854775806 -9223372036854775807) OOME (-9223372036854775806 -9223372036854775807) bottom of long range
(count (range (+ Long/MIN_VALUE 2) Long/MIN_VALUE -1)) 2 2 2 bottom of long range
(range Long/MIN_VALUE Long/MAX_VALUE Long/MAX_VALUE) ArithmeticEx OOME (-9223372036854775806 -1 9223372036854775806) large positive step
(count (range Long/MIN_VALUE Long/MAX_VALUE Long/MAX_VALUE)) ArithmeticEx 0 3 large positive step
(range Long/MAX_VALUE Long/MIN_VALUE Long/MIN_VALUE) ArithmeticEx OOME (9223372036854775807 -1) large negative step
(count (range Long/MAX_VALUE Long/MIN_VALUE Long/MIN_VALUE)) ArithmeticEx 0 2 large negative step
(count (range 0 Long/MAX_VALUE)) overflows to nonsense -2147483648 ArithmeticEx number of values in range > Integer.MAX_VALUE

Cause: There were several bugs, both old and new, in the counting related code for range, particularly around overflow and (introduced in CLJ-1709) coercion error.

Approach: The patched code:

  • Uses only exact values (no double conversion in there)
  • Fixes the algorithm for integer ceiling to be correct
  • Explicitly does overflow-checking calculations when necessary (chunking, counting, and iterator)
  • In the case of overflow, falls back to slower stepping algorithm if necessary (this is only in pathological cases like above)
  • Added many new tests thanks to Andy Fingerhut and Steve Miner.

One particular question is what to do in the case where the count of a range is > Integer.MAX_VALUE. The choices are:
1. Return Integer.MAX_VALUE (Java collection size() solution)
2. Throw ArithmeticOverflowException (since your answer is going to be wrong)
3. Overflow and let bad stuff happen (Clojure 1.6 does this)

The current patch takes approach #2, per Rich.

Performance check:

expr 1.6 beta1 beta2 beta2+patch
(count (range (* 1024 1024))) 63 ms 0 ms 0 ms 0 ms
(reduce + (map inc (range (* 1024 1024)))) 55 ms 35 ms 34 ms 32 ms
(reduce + (map inc (map inc (range (* 1024 1024))))) 74 ms 59 ms 56 ms 54 ms
(count (keep odd? (range (* 1024 1024)))) 77 ms 52 ms 48 ms 49 ms
(transduce (comp (map inc) (map inc)) + (range (* 1024 1024))) n/a 30 ms 26 ms 26 ms
(reduce + 0 (range (* 2048 1024))) 72 ms 29 ms 29 ms 21 ms
(reduce + 0 (rest (range (* 2048 1024)))) 73 ms 29 ms 30 ms 21 ms
(doall (range 0 31)) 1.38 us 0.97 us 0.73 us 0.75 us
(doall (range 0 32)) 1.38 us 0.99 us 0.76 us 0.77 us
(doall (range 0 4096)) 171 us 126 us 125 us 98 us
(into [] (map inc (range 31))) 1.87 us 1.34 us 1.27 us 1.33 us
(into [] (map inc) (range 31)) n/a 0.76 ms 0.76 ms 0.76 ms
(into [] (range 128)) 5.26 us 2.18 us 2.15 us 2.22 us

Patch: clj-1727-4.patch



 Comments   
Comment by Steve Miner [ 07/May/15 10:49 AM ]

It looks like something is overflowing when doing the count calculation. Here's another example:

(count (range (- Long/MAX_VALUE 7)))
-2147483648

Comment by Alex Miller [ 07/May/15 10:54 AM ]

Looks like lack of overflow checking in the chunk logic maybe.

Comment by Alex Miller [ 07/May/15 11:11 AM ]

The example in the description is overflowing in computing nextStart in forceChunk(). That should be fixable, will consider some options.

The example in the comment overflows calculating the count, which is > max int. I'm not sure there actually is a good answer in that particular case. The Java Collection interface expects count() to return Integer.MAX_VALUE in this case (which is bad, but equally bad as every other incorrect answer when the value is not representable in the type).

Comment by Steve Miner [ 07/May/15 11:18 AM ]

LongRange absCount looks suspicious. That double math might be wrong. If the (end - start) is large, the conversion to double loses integer precision. For example, in Clojure:

(- Long/MAX_VALUE (long (double (- Long/MAX_VALUE 1000))))
1023

You might have expected 1000, but the double conversion was not exact. Of course, it works from some values, like exactly Long/MAX_VALUE.

I think it might be safer to restrict the LongRange to the safe bounds, basically inside (bit-shift-right Long/MAX_VALUE 10). Or just use (long Integer/MAX_VALUE) as a reasonable max.

Comment by Andy Fingerhut [ 07/May/15 12:02 PM ]

Some other fun test cases, if the goal is to make LongRange work for entire range of longs:

user=> (take 5 (range (- Long/MAX_VALUE 2) Long/MAX_VALUE))
(9223372036854775805 9223372036854775806 9223372036854775807 -9223372036854775808 -9223372036854775807)
user=> (take 5 (range (- Long/MAX_VALUE 2) Long/MAX_VALUE 7))
(9223372036854775805 -9223372036854775804 -9223372036854775797 -9223372036854775790 -9223372036854775783)
user=> (take 5 (range (- Long/MAX_VALUE 2) Long/MAX_VALUE Long/MAX_VALUE))
(9223372036854775805 -4 9223372036854775803 -6 9223372036854775801)
Comment by Andy Fingerhut [ 07/May/15 1:24 PM ]

Attachment clj-1709-wip-1.patch is intentionally in the wrong format, since it is only intended as a hint of what might be done.

I think using float or double arithmetic for absCount is a very bad idea, given all the subtle roundoff things that could happen there. Exact arithmetic is best.

It has known limitations mentioned in a few comments in the code. There may be unknown limitations, too.

Generative tests that specifically tried to hit the corner cases, e.g. start values often near Long/MIN_VALUE, end values near Long/MAX_VALUE, step values near 0 and the extreme long values, etc. are much more likely to hit any remaining bugs here, but are also very slow to test given the size of the ranges.

Comment by Andy Fingerhut [ 07/May/15 1:36 PM ]

I am liking Steve Miner's suggestion, or something like it, the more I think on it. That is, check a few more conditions on the values of start, end, and step in clojure.core/range, and if they are not met, avoid LongRange and use Range instead. This would put the common cases in LongRange, and only unusual corner cases in the slower Range. At the same time, it could simplify the code for LongRange and help keep it fast.

Comment by Alex Miller [ 07/May/15 4:17 PM ]

The competing constraint here is of course performance. Adding more checks also makes the fast common path slower. By no means ruling it out, but need to consider it.

Comment by Andy Fingerhut [ 07/May/15 5:23 PM ]

Attachment clj-1709-wip-2.patch is a fleshed out version of the approach suggested by Steve Miner – avoid LongRange/create if the long args to range are such that LongRange would misbehave. The example-based tests could be expanded a bit.

Comment by Alex Miller [ 08/May/15 11:03 AM ]

I have a solution for this that does not need additional up-front checks and has (I think) minimal impact on performance. Polishing it...

Comment by Alex Miller [ 08/May/15 11:03 AM ]

Oh, and big thanks Andy for the tests - I will totally steal those, they were very helpful.

Comment by Andy Fingerhut [ 08/May/15 12:12 PM ]

Here are a couple more that I would recommend stealing (implying that the clojure.core/range return value should be equal to the clojure.lang.Range/create version):

(range -1 Long/MAX_VALUE Long/MAX_VALUE)  ; large step values make (step * CHUNK_SIZE) overflow
(range 1 Long/MIN_VALUE Long/MIN_VALUE)
Comment by Andy Fingerhut [ 08/May/15 7:52 PM ]

Alex, clj-1727.patch looks solid to me. Or at least I couldn't find anything wrong with it in 30-40 minutes.

One question: In count(), why fall back to super.count() if the actual value fits in a long but is greater than Integer.MAX_VALUE ? Because we want to be bug-compatible with count() in that case, sometimes returning overflowed values? (see example below). It seems faster and better to return Integer.MAX_VALUE in that case.

user=> *clojure-version*
{:major 1, :minor 6, :incremental 0, :qualifier nil}
user=> (count (range (+ Integer/MAX_VALUE 2)))
-2147483647
Comment by Alex Miller [ 08/May/15 8:46 PM ]

Yeah I went back and forth on that. I actually I was throwing an exception before this version. There is no good answer.

Comment by Steve Miner [ 09/May/15 2:19 PM ]

I did some quick tests with the patch and it looks good. I have to agree with Andy that it would make sense to return Integer.MAX_VALUE for the overflow cases. The super.count() is likely to be so slow that it might as well be an infinite loop. If I'm reading the code correctly, stepOffset is always (step > 0 ? -1 : l). I would expect that to be a very fast computation (especially with a final step) so it's probably not worth caching in a field.

Comment by Alex Miller [ 09/May/15 3:10 PM ]

New -2 patch avoids the new field (~same perf) and changes behavior on count over max int.

Comment by Steve Miner [ 10/May/15 1:06 PM ]

Testing with patch-2. It looks like there are a couple of edge cases that can give negative results where I would have expected Integer/MAX_VALUE (for any overflow of int count).

user=> Integer/MAX_VALUE
2147483647
user=> (count (range Long/MIN_VALUE -2))
2147483647 ;OK
user=> (count (range Long/MIN_VALUE -1))
-2147483648

user=> (count (range 1 Long/MAX_VALUE))
2147483647 ;OK
user=> (count (range 0 Long/MAX_VALUE))
-2147483648

I think that count() should return Integer.MAX_VALUE when there's an ArithmeticException, instead of trying super.count() there.

Comment by Andy Fingerhut [ 10/May/15 2:53 PM ]

Steve, I think that Integer.MAX_VALUE is often an incorrect value for count(), when rangeCount throws an exception. For example, here are some results with patch-2, tweaked to make rangeCount public:

user=> (def x (range 0 1 2))
#'user/x
user=> (. x rangeCount Long/MIN_VALUE Long/MAX_VALUE Long/MAX_VALUE)
ArithmeticException integer overflow  clojure.lang.Numbers.throwIntOverflow (Numbers.java:1501)
user=> (count (range Long/MIN_VALUE Long/MAX_VALUE Long/MAX_VALUE))
3      ; correct value

Also, it seems that the reason it is returning these incorrect values is because super.count() is ASeq.count(), which starts at int 1, and then when it discovers that the remaining thing is a LongRange, which implemented Counted, it calls LongRange#count() and in turn LongRange#rangeCount() on a sequence one shorter. My debug prints in LongRange#count() confused me mightily until I figured out that this is what it is doing.

That also means that expressions like the one below overflow the stack, because of mutually recursive calls between LongRange#count() and ASeq#count().

user=> (count (range Long/MIN_VALUE 10000))
StackOverflowError   java.lang.Exception.<init> (Exception.java:66)

One way to correct the stack overflow issue would be to effectively copy Aseq#count() code into the place where LongRange#count() calls super.count().

If that loop was then modified to check for (i < 0), to detect overflow, and returning Integer.MAX_VALUE if that ever happened, that would also eliminate overflow for LongRange's (but not all ASeq's).

Comment by Steve Miner [ 10/May/15 3:28 PM ]

Good point about the case of a large step potentially causing the exception, in which case the range may still have a reasonable count.

Comment by Alex Miller [ 11/May/15 2:43 AM ]

New -3 patch changes the fallback to use the iterator. The iterator was also not safe from overflow, so I fixed that too.

For counting ranges > Integer/MAX_VALUE, now:

user=> (count (range Long/MIN_VALUE 0))
2147483647
Comment by Andy Fingerhut [ 12/May/15 10:27 AM ]

Re: clj-1727-4.patch
I, for one, will never be filing a bug that (count (range Long/MIN_VALUE Long/MAX_VALUE)) overflows a long

Comment by Steve Miner [ 12/May/15 3:44 PM ]

And I will resist suggesting a count' (ala inc' and dec'). Thanks for fixing these edge cases. The original report came from real life experience when I was trying to fix my own double math problems with TCHECK-67.

Comment by Nicola Mometto [ 05/Jun/15 9:53 AM ]

Patch committed and the ticket marked as resolved but not closed. I'm closing it.





[CLJ-1726] New iterate and cycle impls have delayed computations but don't implement IPending Created: 06/May/15  Updated: 05/Jun/15  Resolved: 12/May/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: regression

Attachments: Text File clj-1726-2.patch     Text File clj-1726.patch    
Patch: Code
Approval: Ok

 Description   

When moving from LazySeqs to the new types we lost this but I don't think we should. Tools like Cursive use this for deciding when and how much to realize from a lazy sequence.

Approach:

  • iterate - The head of an iterate will always have the seed value and return 1 realized value. Subsequent elements will start unrealized and then become realized when the iterate function has been invoked to produce the value.
  • cycle - Returns unrealized if _current has been forced (initially null for all nodes after the first node).

(Note that range and repeat effectively always have their first element realized so I have chosen not to implement IPending - there is no delayed computation pending.)

;; setup
(def i (iterate inc 0))
(def c (cycle [1 2 3]))

user=>  (mapv realized? [i (next i) c (next c)])
[true false true false]
user=> (fnext i)
1
user=> (fnext c)
2
user=> (mapv realized? [i (next i) c (next c)])
[true true true true]

Patch: clj-1726-2.patch



 Comments   
Comment by Fogus [ 08/May/15 9:44 AM ]

There are three things that I like about this patch:

1) The implementations of the isRealize method provide meaning to the somewhat opaque encoding inherent in _current and (less opaque) UNREALIZED_SEED.

2) The use of realized? is generally useful outside of IDE contexts.

3) It's small and easy to grasp.

Comment by Nicola Mometto [ 05/Jun/15 9:53 AM ]

Patch committed and the ticket marked as resolved but not closed. I'm closing it.





[CLJ-1725] Add missing transducer creating functions to clojure.org/transducers Created: 04/May/15  Updated: 04/May/15  Resolved: 04/May/15

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

The following should be added to the list:

  • distinct
  • interpose
  • map-indexed


 Comments   
Comment by Alex Miller [ 04/May/15 3:15 PM ]

Thanks, I'll take care of that.

Comment by A. R [ 04/May/15 4:05 PM ]

Nevermind this comment. Had an old version.

Comment by Alex Miller [ 04/May/15 4:29 PM ]

Added.





[CLJ-1724] Reuse call to seq() in LazySeq/hashcode for else case Created: 04/May/15  Updated: 12/Oct/15  Resolved: 12/Oct/15

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

Type: Enhancement Priority: Minor
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: collections, ft, performance

Attachments: File clj-1724.diff    
Patch: Code
Approval: Ok

 Description   

In LazySeq/hashCode, seq() is called twice for non-empty seqs. First call to seq() can be reused in else case.






[CLJ-1723] NPE with eduction + cat on a collection containing nil Created: 04/May/15  Updated: 05/Jun/15  Resolved: 12/May/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: Moritz Heidkamp Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: transducers

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

 Description   

Using the cat transducer with eduction leads to an NPE when the collection contains at least one collection with more than one item of which at least one is nil. The shortest reproduction case I could come up with is this:

(eduction cat [[nil nil]])

Cause: An expanding transducer (cat, mapcat) puts the expansion on an internal buffer, which is a ConcurrentLinkedQueue. Java Queue impls do not support adding or removing null b/c null is used as a special value in some of the Queue apis.

Approach: Switch from ConcurrentLinkedQueue to LinkedList. LinkedList supports both Queue and other semantics as well and does support nulls (with caveats that that is a bad thing to do if you're using the Queue apis and expecting those special semantics). However, the TransformerIterator usage does not rely on any of that. LinkedList is also obviously not concurrency friendly, but the buffer is only used by a single thread at a time and the volatile field guarantees visibility, so this is fine.

I re-ran some of the perf tests from CLJ-1669 and found the expanding transducer test there (into [] (eduction (map inc) (mapcat range) s50)) went from 27 us to 24 us, so there is a bit of a perf improvement as well.

Patch: clj-1723.patch



 Comments   
Comment by Alex Miller [ 04/May/15 12:03 PM ]

Gah, Java Queues don't allow null. I have some prior work on other impls for this so I'm working on a fix.

Comment by Fogus [ 08/May/15 9:49 AM ]

This is a very straight-forward solution that works and is easy to justify and grasp.

Comment by Nicola Mometto [ 05/Jun/15 9:53 AM ]

Patch committed and the ticket marked as resolved but not closed. I'm closing it.





[CLJ-1722] Typo in the doc string of `with-bindings` Created: 03/May/15  Updated: 31/Jul/15  Resolved: 31/Jul/15

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

Type: Defect Priority: Trivial
Reporter: Blake West Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft

Attachments: Text File fixwithbindingsdocs.patch    
Patch: Code
Approval: Ok

 Description   

The doc string says "the execute body". It should say "then execute body".



 Comments   
Comment by Andy Fingerhut [ 26/May/15 8:47 AM ]

Alex, this one 'falls off the JIRA state chart' since Rich hasn't assigned it a Fix Version. Should Approval be Triaged instead?





[CLJ-1716] IExceptionInfo should print with its ex-data Created: 26/Apr/15  Updated: 12/May/15  Resolved: 12/May/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: Gary Fredericks Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: reader

Attachments: Text File CLJ-1716.patch    
Patch: Code and Test
Approval: Ok

 Description   

The new (for 1.7) data-reader printing format for exceptions does not include the ex-data when relevant:

user> *clojure-version*
{:major 1, :minor 7, :incremental 0, :qualifier "beta2"}
user> (pr (ex-info "msg" {:my-data 42}))
#error {
 :cause "msg"
 :via
 [{:type clojure.lang.ExceptionInfo
   :message "msg"
   :at [clojure.core$ex_info invoke "core.clj" 4591]}]
 :trace
 [[clojure.core$ex_info invoke "core.clj" 4591]
  [user$eval9314 invoke "form-init6701752258113826186.clj" 1]
  ;; ...
]}

Approach: If ExceptionInfo is caught, also print :data key with ex-data. Include :data key for each ExceptionInfo in via.

After:

#error {
 :cause "msg"
 :data {:my-data 42}
 :via
 [{:type clojure.lang.ExceptionInfo
   :message "msg"
   :data {:my-data 42}
   :at [clojure.core$ex_info invoke "core.clj" 4591]}]
 :trace
 [[clojure.core$ex_info invoke "core.clj" 4591]
  [user$eval1 invoke "NO_SOURCE_FILE" 1]
  ;; elided
  ]}

Example with nested ExceptionInfo:

(try 
  (throw (ex-info "cause" {:a :b})) 
  (catch Exception e 
    (throw (ex-info "wrapped" {:c :d} e))))

;; yields:

#error {
 :cause "cause"
 :data {:a :b}
 :via
 [{:type clojure.lang.ExceptionInfo
   :message "wrapped"
   :data {:c :d}
   :at [clojure.core$ex_info invoke "core.clj" 4591]}
  {:type clojure.lang.ExceptionInfo
   :message "cause"
   :data {:a :b}
   :at [clojure.core$ex_info invoke "core.clj" 4591]}]
 :trace
 [[clojure.core$ex_info invoke "core.clj" 4591]
  [user$eval5 invoke "NO_SOURCE_FILE" 4]
  ;; elided
  ]}

Patch: CLJ-1716.patch



 Comments   
Comment by Gary Fredericks [ 26/Apr/15 9:30 PM ]

Added two patch files, the first with tests for the existing code, the second adding the :data key and extra tests for that.

Comment by Alex Miller [ 28/Apr/15 9:55 AM ]

Screening comments:

  • Combine the two patches into a single patch
  • Include :data for all ExceptionInfo in :via (when appropriate) (and leave it where it is)
  • when you git format-patch, throw -W on there to get more context (I think it would help in this case)
  • everything else looks good
Comment by Gary Fredericks [ 29/Apr/15 9:44 AM ]

Cool – I'm assuming "single patch" implies "single commit" since I couldn't find a way to dump two commits into one patch file.

Comment by Alex Miller [ 29/Apr/15 10:08 AM ]

yeah, single commit is what I meant. you can just commit multiple times and format-patch to get a single patch with multiple commits, but would prefer single.

Comment by Gary Fredericks [ 30/Apr/15 9:14 AM ]

Attached CLJ-1716.patch, which has just one commit, and now attaches data both to everything appropriate in :via and at the top level (so the data for the root cause is duplicated).

Added one more test case while I was at it.





[CLJ-1713] Range is not serializable Created: 21/Apr/15  Updated: 24/Apr/15  Resolved: 24/Apr/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: regression

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

 Description   

Range is not serializable once it starts getting chunked.

(import [java.io ByteArrayOutputStream ObjectOutputStream])

(defn- serialize
  "Serializes a single object, returning a byte array."
  [v]
  (with-open [bout (ByteArrayOutputStream.)
              oos (ObjectOutputStream. bout)]
    (.writeObject oos v)
    (.flush oos)
    (.toByteArray bout)))

(serialize (range 10))  ;; works fine
;;=> #object["[B" 0x71c14b1d "[B@71c14b1d"]

(def r (range 50))
(nth r 35)  ;; 35
(serialize r)
;;=> NotSerializableException clojure.lang.LongRange$LongChunk  java.io.ObjectOutputStream.writeObject0 (ObjectOutputStream.java:1181)

Cause: LongChunk is not serializable.

Approach: Make it serializable.

Patch: clj-1713.patch



 Comments   
Comment by Fogus [ 24/Apr/15 9:03 AM ]

This patch couldn't be more trivial. Screened and ready to apply.





[CLJ-1711] Hash map and StructMap cannot be conjoined using "into" Created: 21/Apr/15  Updated: 24/Apr/15  Resolved: 24/Apr/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: Immo Heikkinen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: regression
Environment:

Clojure 1.7.0-beta1


Attachments: Text File clj-1711-fix-structmap-iterator.patch    
Patch: Code and Test
Approval: Ok

 Description   
user=> (defstruct foo :bar)
#'user/foo
user=> (into {} (struct-map foo :bar 1))

IllegalArgumentException Don't know how to create ISeq from: clojure.lang.Keyword  clojure.lang.RT.seqFrom (RT.java:528)

In Clojure 1.6 this returns {:bar 1} as expected.



 Comments   
Comment by Alex Miller [ 21/Apr/15 8:43 AM ]

Thanks for the report - looks like an issue with the iter/reduce path for structmaps which changed in 1.7. Another example yielding same error:

(reduce (fn [acc i] (inc acc)) 0 struct-map)
Comment by Immo Heikkinen [ 22/Apr/15 12:42 AM ]

This turned out to be pretty simple to fix so I gave it a go.

Comment by Alex Miller [ 22/Apr/15 10:51 AM ]

Thanks! I'll take a look at it tomorrow!

Comment by Alex Miller [ 23/Apr/15 11:16 AM ]

Looks great, thanks.





[CLJ-1709] Incorrect range contents and count with step != 1 Created: 18/Apr/15  Updated: 24/Apr/15  Resolved: 24/Apr/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: Nelson Morris Assignee: Devin Walters
Resolution: Completed Votes: 0
Labels: regression
Environment:

clojure 1.7.0-beta1


Attachments: Text File CLJ-1709-1710.patch     Text File clj-1709-2.patch    
Patch: Code and Test
Approval: Ok

 Description   

From https://groups.google.com/d/msg/clojure/dweCm6Bd-vc/atritH--xUEJ.

(range 0 11 2)
;;=> (0 2 4 6 8)
;;Expected: (0 2 4 6 8 10)
(count (range 0 11 2))
;;=> 5
;;Expected: 6

Cause: absCount method in LongRange is not computing count correctly.

Approach: Fix computation in LongRange.absCount() and add a generative test that compares the long and non-long versions of range produce the same ranges.

Screened by: Alex Miller, also verified that generative test failed without the fix.

Patch: clj-1709-2.patch



 Comments   
Comment by Devin Walters [ 18/Apr/15 7:10 PM ]

See attached patch and http://dev.clojure.org/jira/browse/CLJ-1710.

Comment by Alex Miller [ 19/Apr/15 9:07 AM ]

This seems to be related to `clojure.lang.LongRange`. Assuming that, here is a test.check property for being equal to clojure.lang.Range:

(def longrange equals-range               
  (prop/for-all [start gen/int                                                                                                                                            
                 end gen/int                                                                                                                                              
                 step (gen/such-that #(> % 0)                                                                                                                             
                                     gen/nat)]                                                                                                                            
                (= (clojure.lang.Range/create start end step)                                                                                                             
                   (clojure.lang.LongRange/create start end step))))                                                                                                      
                                                                                                                                                                          
(tc/quick-check 100 longrange-equals-range)                                                                                                                               
{:result false, :seed 1429392529363, :failing-size 15, :num-tests 16, :fail [0 15 7], :shrunk {:total-nodes-visited 22, :depth 5, :result false, :smallest [0 4 3]}}
Comment by Alex Miller [ 19/Apr/15 9:08 AM ]

Can we add the test.check property to the patch please? Clojure uses test.check already so this dependency is already taken care of.

Comment by Devin Walters [ 19/Apr/15 4:35 PM ]

@Alex, sure. It looks like transducers is not taking advantage of clojure.test.check's clojure.test integration. Specifically, the use of defspec. Is there a good reason why this is so?

This is what it'd look like:

(defspec longrange-equals-range 100
  (prop/for-all [start gen/int
                 end gen/int
                 step (gen/such-that #(> % 0)
                                     gen/nat)]
                (= (clojure.lang.Range/create start end step)
                   (clojure.lang.LongRange/create start end step))))

When building:

[java] Testing clojure.test-clojure.sequences
[java] {:result true, :num-tests 100, :seed 1429478867534, :test-var longrange-equals-range}

Is this alright? Please advise.

Comment by Devin Walters [ 19/Apr/15 4:44 PM ]

@Alex, I went ahead and did it the way I mentioned above.

Added an updated patch to include generative test to verify LongRange is the same as Range.

Comment by Michael Blume [ 20/Apr/15 12:05 AM ]

@Devin, this may be off-topic, but I'm pretty sure I'm responsible for the lack of defspec in the transducers tests you're talking about, and the reason is simply that I couldn't find a way with defspec to get the clarity of the reporting to the level I wanted.

Comment by Michael Blume [ 20/Apr/15 12:06 AM ]

See CLJ-1621

Comment by Michael Blume [ 20/Apr/15 12:11 AM ]

(I've been meaning for ages to come up with a proposal for test.check itself that would handle that sort of reporting for the user, but never came up with anything I liked enough)

Comment by Devin Walters [ 22/Apr/15 7:47 PM ]

Hey Michael, thanks for the background. I discussed this briefly with Alex at Clojure/West. By the sound of it, using defspec here should be fine. He mentioned he'd take a peek at it.

Comment by Steve Miner [ 23/Apr/15 12:36 PM ]

Regarding the test code, you could use gen/s-pos-int instead of (gen/such-that #(> % 0) gen/nat).

Comment by Alex Miller [ 24/Apr/15 8:20 AM ]

Tweaked one line in the test per Steve's suggestion.

Comment by Nelson Morris [ 24/Apr/15 8:40 AM ]

Since this is still open to suggestions, if I were to write the spec over again I'd probably use (such-that #(not= 0 %) gen/int) for the step. A negative step will produce non-empty lists when end < start, and those might be worth checking. The case to avoid is one that makes an infinite range like (repeat 0 1 0), due to the equality check in the property. A more complicated generator could probably avoid that without a such-that filter, but not sure it would be worth the effort in this case.

Comment by Alex Miller [ 24/Apr/15 9:02 AM ]

Well Rich has now ok'ed it so would prefer no more changes to this patch.





[CLJ-1706] top level conditional splicing ignores all but first element Created: 15/Apr/15  Updated: 21/May/15  Resolved: 21/May/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: 2
Labels: reader

Attachments: Text File 0001-CLJ-1706-Make-top-level-reader-conditional-splicing-.patch     Text File clj-1706-2.patch     Text File clj-1706-3.patch     Text File clj-1706-make-error.patch    
Patch: Code and Test
Approval: Ok

 Description   
user=> (def a (java.io.PushbackReader. (java.io.StringReader. "#?@(:clj [1 2])")))
#'user/a
user=> (read {:read-cond :allow} a)
1
user=> (read {:read-cond :allow} a)
RuntimeException EOF while reading  clojure.lang.Util.runtimeException (Util.java:221)

Currently the reader is stateless (read is a static call) but utilizes a stateful reader (and has a few hooks into compiler/runtime state for autoresolving keywords, etc). If the call into the reader at the top level calls a splicing reader conditional, then only the first one will be returned. The remaining forms are stranded in the pendingForms list and will be lost for subsequent reads.

Approach: Make top level reader conditional splicing an error:

user=> (read-string {:read-cond :allow} "#?@(:clj [1 2])")
IllegalStateException Reader conditional splicing not allowed at the top level.  clojure.lang.LispReader.read (LispReader.java:200)

Patch: clj-1706-2.patch

Alternatives:

1. Make top-level reader conditional splicing an error and throw an exception. (SELECTED)

2. Allow the caller to pass in a stateful collection to catch or return the pendingForms. This changes the effective calling API for the reader. You would only need to do this in the cases where reader conditionals were allowed/preserved.

3. Add a static (or threadlocal?) pendingForms attribute to the reader to capture the pendingForms across calls. A static field would have concurrency issues - anyone using the reader across threads would get cross-talk in this field. The pendingForms could be threadlocal which would probably achieve separation in the majority of cases, but also creates a number of lifecycle questions about those forms. When do they get cleared or reset? What happens if reading the same reader happens across threads? Another option would be an identity map keyed by reader instance - would need to be careful about lifecycle management and clean up, as it's basically a cache.

4. Add more state into the reader itself to capture the pendingForms. The reader interfaces and hierarchy would be affected. This would allow the reader to stop passing the pendingForms around inside but modifies the interface in other ways. Again, this would only be needed for the specific case where reader conditionals are allowed so other uses could continue to work as is?

5. If read is going to exit with pendingForms on the stack, they could be printed and pushed back onto the reader. This adds new read/print roundtrip requirements on things at the top level of reader conditionals that didn't exist before.

6. Wrap spliced forms at the top level in a `do`. This seems to violate the intention of splicing reader conditional to read as spliced since it is not the same as if those forms were placed separately in the input stream.



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

pulling into 1.7 so we can discuss

Comment by Alex Miller [ 24/Apr/15 11:18 AM ]

Compiler.load() makes calls into LispReader.read() (static call). If the reader reads a top-level splicing conditional read form, that will read the entire form, then return the first spliced element. when LispReader.read() returns, the list carrying the other pending forms is lost.

I see two options:

1) Allow the compiler to call the LispReader with a mutable pendingForms list, basically maintaining that state across the static calls from outside the reader. makes the calling model more complicated and exposes the internal details of the pendingform stuff, but is probably the smaller change.

2) Enhance the LineNumberingPushbackReader in some way to remember the pending forms. This would probably allow us to remove the pending form stuff carried around throughout the LispReader and retain the existing (sensible) api. Much bigger change but probably better direction.

Comment by Nicola Mometto [ 24/Apr/15 11:24 AM ]

What about simply disallowing cond-splicing when top level?
Both proposed options are breaking changes since read currently only requires a java.io.PushbackReader

Comment by Alex Miller [ 24/Apr/15 11:42 AM ]

We want to allow top-level cond-splicing.

Comment by Nicola Mometto [ 24/Apr/15 11:52 AM ]

Would automatically wrapping a top-level cond-splicing in a (do ..) form be out of the question?

I'm personally opposed to supporting this feature as it would change the contract of c.c/read, complicate the implementation of LineNumberingPushbackReader or LispReader and complicate significantly the implementaion of tools.reader's reader types, for no significant benefit.
Is it really that important to be able to write

#~@(:clj [1 2])

rather than

(do #~@(:clj [1 2]))

?

Comment by Rich Hickey [ 18/May/15 10:10 AM ]

Please "Make top-level reader conditional splicing an error and throw an exception" for now.

Comment by Nicola Mometto [ 19/May/15 8:50 AM ]

Might be too late since Rich already gave the OK but the proposed patch doesn't prevent single-element top level conditional splicing forms.
e.g

;; this fails
#~@(:clj [1 2])
;; this works
#~@(:clj [1])

Is this intended?

Comment by Alex Miller [ 19/May/15 11:21 AM ]

New -2 patch catches reader conditional splice of 0 or 1 element.

Comment by Nicola Mometto [ 19/May/15 11:59 AM ]

Attached alternative patch that is less intrusive than clj-1706-2.patch

Comment by Alex Miller [ 19/May/15 2:54 PM ]

clj-1706-3.patch is identical to 0001-CLJ-1706Make-top-level-reader-conditional-splicing.patch but with one whitespace change reverted. Marking latest as screened.

Comment by Alex Miller [ 20/May/15 8:38 AM ]

Rich didn't like the dynvar in -3, so switching back to -2.





[CLJ-1703] Pretty print #error data Created: 14/Apr/15  Updated: 24/Apr/15  Resolved: 24/Apr/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: Alex Miller
Resolution: Completed Votes: 0
Labels: print

Attachments: Text File clj-1703-2.patch     Text File clj-1703.patch    
Patch: Code
Approval: Ok

 Description   

Some of the work we were doing re socket repls got pushed out but it would still be nice to expose the pretty printed #error formatting as the current version is very hard to read.

1.7.0-beta1:

user=> *99

CompilerException java.lang.RuntimeException: Unable to resolve symbol: *99 in this context, compiling:(/private/var/folders/7r/_1fj0f517rgcxwx79mn79mfc0000gn/T/form-init8696775124159270468.clj:1:1263)
user=> (prn *e)
#error{:cause "Unable to resolve symbol: *99 in this context", :via [{:type clojure.lang.Compiler$CompilerException, :message "java.lang.RuntimeException: Unable to resolve symbol: *99 in this context, compiling:(/private/var/folders/7r/_1fj0f517rgcxwx79mn79mfc0000gn/T/form-init8696775124159270468.clj:1:1263)", :at [clojure.lang.Compiler analyze "Compiler.java" 6543]} {:type java.lang.RuntimeException, :message "Unable to resolve symbol: *99 in this context", :at [clojure.lang.Util runtimeException "Util.java" 221]}], :trace [[clojure.lang.Util runtimeException "Util.java" 221] [clojure.lang.Compiler resolveIn "Compiler.java" 7029] [clojure.lang.Compiler resolve "Compiler.java" 6973] [clojure.lang.Compiler analyzeSymbol "Compiler.java" 6934] [clojure.lang.Compiler analyze "Compiler.java" 6506] [clojure.lang.Compiler analyze "Compiler.java" 6485] [clojure.lang.Compiler eval "Compiler.java" 6796] [clojure.lang.Compiler eval "Compiler.java" 6755] [clojure.core$eval invoke "core.clj" 3082] [clojure.main$repl$read_eval_print__7057$fn__7060 invoke "main.clj" 240] [clojure.main$repl$read_eval_print__7057 invoke "main.clj" 240] [clojure.main$repl$fn__7066 invoke "main.clj" 258] [clojure.main$repl doInvoke "main.clj" 258] [clojure.lang.RestFn invoke "RestFn.java" 1523] [clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__599 invoke "interruptible_eval.clj" 53] [clojure.lang.AFn applyToHelper "AFn.java" 152] [clojure.lang.AFn applyTo "AFn.java" 144] [clojure.core$apply invoke "core.clj" 628] [clojure.core$with_bindings_STAR_ doInvoke "core.clj" 1866] [clojure.lang.RestFn invoke "RestFn.java" 425] [clojure.tools.nrepl.middleware.interruptible_eval$evaluate invoke "interruptible_eval.clj" 51] [clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn__641$fn__644 invoke "interruptible_eval.clj" 183] [clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__634 invoke "interruptible_eval.clj" 152] [clojure.lang.AFn run "AFn.java" 22] [java.util.concurrent.ThreadPoolExecutor runWorker "ThreadPoolExecutor.java" 1142] [java.util.concurrent.ThreadPoolExecutor$Worker run "ThreadPoolExecutor.java" 617] [java.lang.Thread run "Thread.java" 744]]}

Approach: Do some minimal formatting of the #error data, should be pretty close to (pprint (Throwable->map *e)).

w/patch:

user=> (prn *e)
#error {
 :cause "Unable to resolve symbol: *99 in this context"
 :via
 [{:type clojure.lang.Compiler$CompilerException
   :message "java.lang.RuntimeException: Unable to resolve symbol: *99 in this context, compiling:(NO_SOURCE_PATH:0:0)"
   :at [clojure.lang.Compiler analyze "Compiler.java" 6543]}
  {:type java.lang.RuntimeException
   :message "Unable to resolve symbol: *99 in this context"
   :at [clojure.lang.Util runtimeException "Util.java" 221]}]
 :trace
 [[clojure.lang.Util runtimeException "Util.java" 221]
  [clojure.lang.Compiler resolveIn "Compiler.java" 7029]
  [clojure.lang.Compiler resolve "Compiler.java" 6973]
  [clojure.lang.Compiler analyzeSymbol "Compiler.java" 6934]
  [clojure.lang.Compiler analyze "Compiler.java" 6506]
  [clojure.lang.Compiler analyze "Compiler.java" 6485]
  [clojure.lang.Compiler eval "Compiler.java" 6796]
  [clojure.lang.Compiler eval "Compiler.java" 6755]
  [clojure.core$eval invoke "core.clj" 3079]
  [clojure.main$repl$read_eval_print__7093$fn__7096 invoke "main.clj" 240]
  [clojure.main$repl$read_eval_print__7093 invoke "main.clj" 240]
  [clojure.main$repl$fn__7102 invoke "main.clj" 258]
  [clojure.main$repl doInvoke "main.clj" 258]
  [clojure.lang.RestFn invoke "RestFn.java" 421]
  [clojure.main$repl_opt invoke "main.clj" 324]
  [clojure.main$main doInvoke "main.clj" 422]
  [clojure.lang.RestFn invoke "RestFn.java" 397]
  [clojure.lang.Var invoke "Var.java" 375]
  [clojure.lang.AFn applyToHelper "AFn.java" 152]
  [clojure.lang.Var applyTo "Var.java" 700]
  [clojure.main main "main.java" 37]]}

Patch: clj-1703-2.patch



 Comments   
Comment by Rich Hickey [ 17/Apr/15 9:48 AM ]

Can we please put the kv pairs of via each on their own line?





[CLJ-1700] REPL evaluation of conditional reader forms fails Created: 12/Apr/15  Updated: 24/Apr/15  Resolved: 24/Apr/15

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

Type: Defect Priority: Minor
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Completed Votes: 3
Labels: reader
Environment:

1.7-beta1


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

 Description   

When using reader conditionals, evaluating a reader conditional in a vanilla command-line REPL (not nRepl or anything like that) results in a "Conditional read not allowed" error message.

Loading the whole file with load-file works as expected.

This breaks the very normal workflow of eval-ing forms from a *.cljc file in a Clojure repl using (e.g.) inferior lisp.

Approach: clojure.main/repl (also used by swank I think) enables reader conditionals at the REPL.

Patch: clj-1700.patch



 Comments   
Comment by Colin Jones [ 21/Apr/15 12:53 AM ]

Looks/works great for me - I quite literally wrote the exact same patch before talking w/ Luke today about this.





[CLJ-1699] data_readers hard coded to .clj extension, should be extended to .cljc Created: 10/Apr/15  Updated: 24/Apr/15  Resolved: 24/Apr/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: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: reader

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

 Description   

Currently using data_readers in ClojureScript is difficult because the extensions are not available to both compile time and runtime as they are in Clojure. This is fairly straightforward to remedy now given the presence of conditional reading - simply supply data_readers.cljc.

Approach: Find and read both data_readers.clj and data_readers.cljc. For cljc, allow reader conditionals.

Alternative: Another option would be to just allow reader conditionals on the existing data_readers.clj file. That's a simpler patch but possibly confusing given that conditionals are only available in .cljc files right now.

Patch: clj-1699.patch - tested with a variety of manual tests



 Comments   
Comment by David Nolen [ 10/Apr/15 4:23 PM ]

This could be solved trivially by concatenated data_readers.cljc resources to the return value of clojure.core/data-reader-urls.





[CLJ-1698] conditional reading bugs Created: 10/Apr/15  Updated: 24/Apr/15  Resolved: 24/Apr/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: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 3
Labels: reader

Attachments: Text File 0001-CLJ-1698-fix-conditional-reading-bugs.patch    
Patch: Code and Test
Approval: Ok

 Description   

Bugs in conditional reading:

(ns bug)

#?(:cljs {'a 1 'b 2})

#?(:cljs (let [{{b :b} :a {d :d} :c} {}]))

Requiring / loading this file at the REPL results in the following exception:

CompilerException java.lang.IllegalArgumentException: Duplicate key: null, compiling:
	clojure.lang.Compiler.load (Compiler.java:7244)
	clojure.lang.RT.loadResourceScript (RT.java:371)
	clojure.lang.RT.loadResourceScript (RT.java:362)
	clojure.lang.RT.load (RT.java:446)
	clojure.lang.RT.load (RT.java:412)
	clojure.core/load/fn--5427 (core.clj:5862)
	clojure.core/load (core.clj:5861)
	clojure.core/load-one (core.clj:5667)
	clojure.core/load-lib/fn--5376 (core.clj:5707)
	clojure.core/load-lib (core.clj:5706)
	clojure.core/apply (core.clj:630)
	clojure.core/load-libs (core.clj:5745)
Caused by:
IllegalArgumentException Duplicate key: null
	clojure.lang.PersistentArrayMap.createWithCheck (PersistentArrayMap.java:70)
	clojure.lang.RT.map (RT.java:1537)
	clojure.lang.LispReader$MapReader.invoke (LispReader.java:1152)
	clojure.lang.LispReader.read (LispReader.java:255)
	clojure.lang.LispReader.access$800 (LispReader.java:40)
	clojure.lang.LispReader$ConditionalReader.readCondDelimited (LispReader.java:1376)
	clojure.lang.LispReader$ConditionalReader.invoke (LispReader.java:1448)
	clojure.lang.LispReader$DispatchReader.invoke (LispReader.java:684)
	clojure.lang.LispReader.read (LispReader.java:255)
	clojure.lang.LispReader.readDelimitedList (LispReader.java:1191)
	clojure.lang.LispReader$ListReader.invoke (LispReader.java:1040)
	clojure.lang.LispReader.read (LispReader.java:255)
(ns bug)

(def m #?(:cljs ^{:a :b} {}
          :clj  ^{:a :b} {}))
CompilerException java.lang.IllegalArgumentException: Metadata must be Symbol,Keyword,String or Map, compiling:(bug.cljc:3:25)
	clojure.lang.Compiler.load (Compiler.java:7244)
	clojure.lang.RT.loadResourceScript (RT.java:371)
	clojure.lang.RT.loadResourceScript (RT.java:362)
	clojure.lang.RT.load (RT.java:446)
	clojure.lang.RT.load (RT.java:412)
	clojure.core/load/fn--5427 (core.clj:5862)
	clojure.core/load (core.clj:5861)
	clojure.core/load-one (core.clj:5667)
	clojure.core/load-lib/fn--5376 (core.clj:5707)
	clojure.core/load-lib (core.clj:5706)
	clojure.core/apply (core.clj:630)
	clojure.core/load-libs (core.clj:5745)
Caused by:
IllegalArgumentException Metadata must be Symbol,Keyword,String or Map
	clojure.lang.LispReader$MetaReader.invoke (LispReader.java:790)
	clojure.lang.LispReader.read (LispReader.java:255)
	clojure.lang.LispReader.access$800 (LispReader.java:40)
	clojure.lang.LispReader$ConditionalReader.readCondDelimited (LispReader.java:1376)
	clojure.lang.LispReader$ConditionalReader.invoke (LispReader.java:1448)
	clojure.lang.LispReader$DispatchReader.invoke (LispReader.java:684)
	clojure.lang.LispReader.read (LispReader.java:255)
	clojure.lang.LispReader.readDelimitedList (LispReader.java:1191)
	clojure.lang.LispReader$ListReader.invoke (LispReader.java:1040)
	clojure.lang.LispReader.read (LispReader.java:255)
	clojure.lang.LispReader.read (LispReader.java:195)
	clojure.lang.Compiler.load (Compiler.java:7232)
(ns bug)

#?(:cljs {:a #_:b :c})
CompilerException java.lang.RuntimeException: Map literal must contain an even number of forms

Cause: Not properly handling suppress-read flag.

Patch: 0001-CLJ-1698-fix-conditional-reading-bugs.patch



 Comments   
Comment by Alex Miller [ 11/Apr/15 5:49 AM ]

For Clojure REPL repro:

(def opts {:features #{:clj} :read-cond :allow})
(read-string opts "#?(:cljs {'a 1 'b 2})")
(read-string opts "#?(:cljs (let [{{b :b} :a {d :d} :c} {}]))")
(read-string opts "(def m #?(:cljs ^{:a :b} {} :clj  ^{:a :b} {}))")
(read-string opts "(def m #?(:cljs ^{:a :b} {} :clj ^{:a :b} {}))")
(read-string opts "#?(:cljs {:a #_:b :c}")




[CLJ-1697] Primitive lexical bindings trigger compile time exceptions under certain conditions Created: 07/Apr/15  Updated: 07/Apr/15  Resolved: 07/Apr/15

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

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

Java 8, Clojure 1.7.0-alpha6



 Description   

A compile time exception is thrown when a value returned from a function with a primitive return signature is lexically bound and used in subsequent bindings within the same let expression.

Example code which exhibits the behavior is available as a gist here:
https://gist.github.com/aamedina/82fee074fb2fb398d4e1

Relevant stack traces are referenced in the comments.



 Comments   
Comment by Adrian Medina [ 07/Apr/15 6:23 PM ]

This does not seem to be a bug. The primitive type hint needs to be precede every argument vector, unlike other return type hinting used to elide reflection.





[CLJ-1695] Variadic vector-of overload has poor performance Created: 03/Apr/15  Updated: 10/Apr/15  Resolved: 10/Apr/15

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

Type: Enhancement Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: performance

Attachments: Text File clj-1695-2.patch     Text File clj-1695.patch    
Patch: Code
Approval: Ok

 Description   
(time (do (apply vector-of :double (repeat 100000 0))
                nil))

Times after a few runs ~335 ms.

(time (do (into (vector-of :double) (repeat 100000 0))
                nil))

Times after a few runs ~5 ms.

Cause: The variadic case for vector-of is missing a type hint and uses reflection - this will be seen in any call to vector-of with more than 4 elements.

Approach: Switch interop call to instead use conj - there is no reason to be using interop here over standard conj on a vector. The after time for the first case above is 6-9 ms depending on GC (the fast reduce path in repeat reduces gc variability I think).

Patch: clj-1695-2.patch



 Comments   
Comment by Leon Grapenthin [ 03/Apr/15 4:57 PM ]

I thought seqs were passed pretty much "as is" with apply. E.g.:

(time (do (apply (fn [& args]
(into (vector-of :double) args))
(repeat 100000 0))
nil))

performs as fast as (into (vector-of :double) (repeat 100000 0)

Comment by Alex Miller [ 06/Apr/15 9:51 AM ]

I'm going to see if we can include this in 1.7, no guarantees. For now a workaround is any use of vector-of that creates, then fills the vector separately as you are doing with into.

The into case actually will get an additional benefit in 1.7 for sources that can reduce themselves faster (repeat happens to be one of those).

Comment by Michael Blume [ 06/Apr/15 11:02 AM ]

If we're adding a type-hint anyway, wouldn't it be more effective to type-hint the return value of vector-of?

Comment by Alex Miller [ 06/Apr/15 11:10 AM ]

I went back-and-forth on that but it didn't seem like that any other code could benefit from the return type hint?

Re-thinking, maybe the hint should actually be more generic and hint to clojure.lang.IPersistentCollection instead.

Comment by Michael Blume [ 06/Apr/15 11:15 AM ]

Actually I'm wrong, type-hinting the return of vector-of (as either Vec or IPersistentCollection) doesn't remove the reflection.

Comment by Alex Miller [ 06/Apr/15 11:37 AM ]

Attached new patch with more generic typehint, placed more specifically where it's needed.

Comment by Alex Miller [ 10/Apr/15 9:52 AM ]

Switched interop call to just





[CLJ-1694] cycle is too eager Created: 03/Apr/15  Updated: 10/Apr/15  Resolved: 10/Apr/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: regression

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

 Description   

Same as CLJ-1692, cycle realizes one element ahead in next(), which is different than before CLJ-1603 was applied:

(take 2 (cycle (map #(/ 42 %) '(2 1 0))))

Approach: Delay realization until first is called.

Patch: clj-1694.patch



 Comments   
Comment by Alex Miller [ 03/Apr/15 2:56 PM ]

Actually, the behavior here is no different vs before.

(take 2 (cycle (map #(/ 42 %) (range 31 -1 -1))))
;; exception

(take 2 (cycle (map #(/ 42 %) (range 32 -1 -1))))
;; works

Just a side effect of chunking and afaict not any different than before.

Comment by Nicola Mometto [ 03/Apr/15 3:21 PM ]

The regression does exist, just a bad example.
Here's a valid one:

Clojure 1.7.0-master-SNAPSHOT
user=> (take 2 (cycle (map #(/ 42 %) '(2 1 0))))
ArithmeticException Divide by zero  clojure.lang.Numbers.divide (Numbers.java:158)
Clojure 1.6.0
user=> (take 2 (cycle (map #(/ 42 %) '(2 1 0))))
(21 42)




[CLJ-1692] Iterate is too eager Created: 01/Apr/15  Updated: 10/Apr/15  Resolved: 10/Apr/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: Ambrose Bonnaire-Sergeant Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: regression

Attachments: Text File clj-1692-2.patch     Text File clj-1692-3.patch     Text File clj1692.patch    
Patch: Code and Test
Approval: Ok

 Description   

1.7's iterate calls its function one extra time than in 1.6

;; clojure 1.6
user=> (take 2 (iterate zero? 0))
(0 true)

;; clojure 1.7-alpha6
user=> (take 2 (iterate zero? 0))
ClassCastException java.lang.Boolean cannot be cast to java.lang.Number  clojure.lang.Numbers.isZero (Numbers.java:92)

This is because Iterate.java calls its function in its "next" method rather than in its "first" method.

Approach: Iterate now holds a prevSeed that will allow computation of a lazily computed seed (the first of the iterate sequence). The very first node will be initialized with the initial seed. All other uses of seed need to ensure first is called to force realization before using seed.

Patch: clj-1692-3.patch

  • note there were two copies of test-iterate so one is removed in the test, allowing the more expansive one to be called.


 Comments   
Comment by Alex Miller [ 02/Apr/15 9:31 AM ]

What about something like clj-1692-2.patch instead? Seems simpler to me. Also, why did the first patch remove the iterate tests?

Comment by Nicola Mometto [ 02/Apr/15 9:55 AM ]

Alex, unless I'm reading it wrong, your patch addresses the symptom of this bug rather than its cause (iterate is now eager than it was before), I personally prefer Ambrose's approach.

WRT removing the iterate tests, it looks like test-iterate is duplicated at line 781 and at line 970 of sequences.clj, Ambrose's patch just removes the duplicate test.

Comment by Ambrose Bonnaire-Sergeant [ 02/Apr/15 10:57 AM ]

To be clear, removing the redundant deftest actually reveals several tests that were previously hidden.

Comment by Alex Miller [ 02/Apr/15 12:01 PM ]

Ok, fair points. I renamed some fields in the first patch to (in my opinion) better reflect their role. There were also several bugs related to using seed without guaranteeing it's computation, for example both of these threw exceptions:

(first (next (next (iterate inc 0))))
(into [] (take 3) (next (iterate inc 0)))

I added those tests as well. I re-ran the perf test from CLJ-1603 and there is definitely some degradation on (doall (take 1000 (iterate inc 0))) from 75 us to 85 us, but that's still a significant win over the prior version.

Comment by Fogus [ 03/Apr/15 1:36 PM ]

I think patch-3 is sound, and I suspect that only Ambrose would have caught it. Thanks!

Comment by Ambrose Bonnaire-Sergeant [ 03/Apr/15 1:48 PM ]

This issue was not caught by Typed Clojure, rather the peculiarities of its implementation https://github.com/clojure/core.typed/commit/1027f792accdc3e5da3475745da0106f0ad5e1cc#diff-eec0f851200aca22af17269fcab94719L1759

I was using iterate to dig down a structure, and was being very careful to only `take` exactly enough.





[CLJ-1691] Occasional failure of seq-and-transducer test Created: 01/Apr/15  Updated: 10/Apr/15  Resolved: 10/Apr/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: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File build-failure2.txt     Text File clj-1691.patch    
Patch: Code
Approval: Ok

 Description   

I started a 'mvn clean ; mvn test' build loop on a machine yesterday. I saw 2 failures of the test seq-and-transducer in transducers.clj out of 520 runs (and no other failures). The first failure message appears below. The second is in the attachment build-fail2.txt. I haven't yet analyzed them carefully to see whether the generated test is useful.

[java] Testing clojure.test-clojure.transducers
     [java] 
     [java] FAIL in (seq-and-transducer) (transducers.clj:143)
     [java] {:coll [0 0],
     [java]  :actions
     [java]  (->>
     [java]   coll
     [java]   (partition-all 1)
     [java]   (interpose 1)
     [java]   dedupe
     [java]   (remove empty?)
     [java]   (take-while empty?)),
     [java]  :s
     [java]  #error{:cause "Don't know how to create ISeq from: java.lang.Long", :via [{:type java.lang.IllegalArgumentException, :message "Don't know how to create ISeq from: java.lang.Long", :at [clojure.lang.RT seqFrom "RT.java" 528]}], :trace [[clojure.lang.RT seqFrom "RT.java" 528] [clojure.lang.RT seq "RT.java" 509] [clojure.core$seq__4106 invoke "core.clj" 135] [clojure.core$empty_QMARK_ invoke "core.clj" 5946] [clojure.core$complement$fn__4358 invoke "core.clj" 1374] [clojure.core$filter$fn__4558 invoke "core.clj" 2683] [clojure.lang.LazySeq sval "LazySeq.java" 40] [clojure.lang.LazySeq seq "LazySeq.java" 49] [clojure.lang.RT seq "RT.java" 507] [clojure.core$seq__4106 invoke "core.clj" 135] [clojure.core$take_while$fn__4582 invoke "core.clj" 2771] [clojure.lang.LazySeq sval "LazySeq.java" 40] [clojure.lang.LazySeq seq "LazySeq.java" 49] [clojure.lang.RT seq "RT.java" 507] [clojure.core$seq__4106 invoke "core.clj" 135] [clojure.core$dorun invoke "core.clj" 3010] [clojure.core$doall invoke "core.clj" 3026] [clojure.test_clojure.transducers$apply_as_seq invoke "transducers.clj" 82] [clojure.test_clojure.transducers$build_results$fn__26512 invoke "transducers.clj" 115] [clojure.test_clojure.transducers$build_results invoke "transducers.clj" 115] [clojure.test_clojure.transducers$fn__26524 invoke "transducers.clj" 130] [clojure.test.check.rose_tree$fmap invoke "rose_tree.clj" 46] [clojure.core$partial$fn__4505 invoke "core.clj" 2491] [clojure.core$map$fn__4531 invoke "core.clj" 2622] [clojure.lang.LazySeq sval "LazySeq.java" 40] [clojure.lang.LazySeq seq "LazySeq.java" 49] [clojure.lang.RT seq "RT.java" 507] [clojure.core$seq__4106 invoke "core.clj" 135] [clojure.test.check.rose_tree$permutations$iter__26065__26069$fn__26070$iter__26089__26093$fn__26094 invoke "rose_tree.clj" 73] [clojure.lang.LazySeq sval "LazySeq.java" 40] [clojure.lang.LazySeq seq "LazySeq.java" 49] [clojure.lang.RT seq "RT.java" 507] [clojure.core$seq__4106 invoke "core.clj" 135] [clojure.core$map$fn__4531 invoke "core.clj" 2614] [clojure.lang.LazySeq sval "LazySeq.java" 40] [clojure.lang.LazySeq seq "LazySeq.java" 49] [clojure.lang.RT seq "RT.java" 507] [clojure.core$seq__4106 invoke "core.clj" 135] [clojure.core$map$fn__4531 invoke "core.clj" 2614] [clojure.lang.LazySeq sval "LazySeq.java" 40] [clojure.lang.LazySeq seq "LazySeq.java" 49] [clojure.lang.Cons next "Cons.java" 39] [clojure.lang.RT next "RT.java" 674] [clojure.core$next__4090 invoke "core.clj" 64] [clojure.core$nthnext invoke "core.clj" 3039] [clojure.test.check$shrink_loop invoke "check.clj" 94] [clojure.test.check$failure invoke "check.clj" 121] [clojure.test.check$quick_check doInvoke "check.clj" 65] [clojure.lang.RestFn invoke "RestFn.java" 425] [clojure.test_clojure.transducers$fn__26531 invoke "transducers.clj" 139] [clojure.test$test_var$fn__7628 invoke "test.clj" 704] [clojure.test$test_var invoke "test.clj" 704] [clojure.test$test_vars$fn__7650$fn__7655 invoke "test.clj" 722] [clojure.test$default_fixture invoke "test.clj" 674] [clojure.test$test_vars$fn__7650 invoke "test.clj" 722] [clojure.test$default_fixture invoke "test.clj" 674] [clojure.test$test_vars invoke "test.clj" 718] [clojure.test$test_all_vars invoke "test.clj" 728] [clojure.test$test_ns invoke "test.clj" 747] [clojure.core$map$fn__4531 invoke "core.clj" 2622] [clojure.lang.LazySeq sval "LazySeq.java" 40] [clojure.lang.LazySeq seq "LazySeq.java" 49] [clojure.lang.Cons next "Cons.java" 39] [clojure.lang.RT next "RT.java" 674] [clojure.core$next__4090 invoke "core.clj" 64] [clojure.core$reduce1 invoke "core.clj" 907] [clojure.core$reduce1 invoke "core.clj" 898] [clojure.core$merge_with doInvoke "core.clj" 2937] [clojure.lang.RestFn applyTo "RestFn.java" 139] [clojure.core$apply invoke "core.clj" 630] [clojure.test$run_tests doInvoke "test.clj" 762] [clojure.lang.RestFn applyTo "RestFn.java" 137] [clojure.core$apply invoke "core.clj" 628] [user$eval28346 invoke "run_test.clj" 7] [clojure.lang.Compiler eval "Compiler.java" 6792] [clojure.lang.Compiler load "Compiler.java" 7237] [clojure.lang.Compiler loadFile "Compiler.java" 7175] [clojure.main$load_script invoke "main.clj" 275] [clojure.main$script_opt invoke "main.clj" 337] [clojure.main$main doInvoke "main.clj" 421] [clojure.lang.RestFn invoke "RestFn.java" 408] [clojure.lang.Var invoke "Var.java" 379] [clojure.lang.AFn applyToHelper "AFn.java" 154] [clojure.lang.Var applyTo "Var.java" 700] [clojure.main main "main.java" 37]]},
     [java]  :xs (),
     [java]  :xi [],
     [java]  :xe [],
     [java]  :xt []}
     [java] 
     [java] expected: (:result res)
     [java]   actual: false


 Comments   
Comment by Ghadi Shayban [ 01/Apr/15 9:34 AM ]

Both failures seem like a test script problem: an 'empty?' check after an interpose of a scalar long.

Mildly surprised that these didn't show up earlier.

Comment by Nicola Mometto [ 01/Apr/15 9:48 AM ]

Looks like this is caused by the chunkedness of keep:

;; lists are not chunked
user=> (->> '([1] 1) (keep empty?) first)
()
;; vector seqs are chunked
user=> (->> '[[1] 1] (keep empty?) first)
IllegalArgumentException Don't know how to create ISeq from: java.lang.Long  clojure.lang.RT.seqFrom (RT.java:528)

In the non-chunked example (mimicking how transducers work), the call to (empty? 1) never gets executed since only the first value is required, on the chunked example otoh (empty? 1) gets invoked causing the exception.





[CLJ-1689] Provide a default kv-reduce path for IPersistentVector Created: 31/Mar/15  Updated: 11/Jan/16  Resolved: 11/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Michael Blume Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1689-v1.patch     Text File clj-1689-v2.patch     Text File clj-1689-v3.patch    
Patch: Code

 Description   

Same as we do with IPersistentMap, make life a bit easier for implementors of IPersistentVector (though they can and should still provide a fast path)



 Comments   
Comment by Mike Anderson [ 21/Jul/15 1:07 AM ]

Would it potentially be better (or as an addition?) to add "reduce" and "kvreduce" methods to APersistentVector in the Java source?

This would allow better performance on the fast path, including the ability to make use of specialised implementations in subclasses (e.g. Tuples) in cases where these can do much better than the use of a volatile! box.

Comment by Ghadi Shayban [ 09/Jan/16 4:46 PM ]

Vectors already implement IKVReduce, though kv-reduce dispatch can be a bit cleaner. Please close this ticket.

Comment by Alex Miller [ 11/Jan/16 8:38 AM ]

PersistentVector implements IKVRreduce





[CLJ-1685] :eof option in clojure.core/read not handled properly Created: 29/Mar/15  Updated: 10/Apr/15  Resolved: 10/Apr/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: Adrian Medina Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: reader

Attachments: Text File 0001-CLJ-1685-correctly-handle-eof-option-in-read-read-st.patch     Text File 0001-CLJ-1685-correctly-handle-eof-option-in-read-read-st-v2.patch     Text File clj-1685-v3.patch    
Patch: Code and Test
Approval: Ok

 Description   

Example form which exhibits the behavior:

(read {:read-cond :allow :eof (Object.)} input)

When EOF is reached in the stream, instead of returning the :eof value specified the boolean value true is always returned instead. If you omit :eof from the option map given to clojure.core/read, false is consistently returned and no EOF error is thrown.
Patch: 0001-CLJ-1685-correctly-handle-eof-option-in-read-read-st-v2.patch

Note: Currently

(read {} stream)
behaves like
(read {:eof nil} stream)
rather than
(read stream)
, the proposed patch makes it believe like
(read {:eof :eofthrow} input)
, the proposed patch changes this so that the default behaviour is always to throw on eof unless a :eof option is explicitly included in the read opts.

Patch: clj-1685-v3.patch

Screened by: Alex Miller



 Comments   
Comment by Nicola Mometto [ 29/Mar/15 2:38 PM ]

Attached patch fixes the issue for both read and read-string

Comment by Andy Fingerhut [ 29/Mar/15 2:47 PM ]

Never try to race Nicola to a patch when he is on the task Thanks, Nicola.

Comment by Nicola Mometto [ 30/Mar/15 8:20 AM ]

Alex, currently calls to read/read-string with an empty options map behave as if {:eof nil} was passed, thus

user=> (read-string "")
RuntimeException EOF while reading  clojure.lang.Util.runtimeException (Util.java:221)
user=> (read-string {} "")
nil

i.e, :eof defaults to nil.
Is this intended? if not, the attached patch 0001-CLJ-1685-correctly-handle-eof-option-in-read-read-st-v2.patch
fixes this issue and changes the behaviour of read/read-string to default to :eofthrow rather than to nil

Comment by Alex Miller [ 06/Apr/15 11:30 AM ]

The -v3 patch is identical to -v2, but adds one clarifying docstring addition.





[CLJ-1684] Transducer eduction test is wrong Created: 27/Mar/15  Updated: 31/Mar/15  Resolved: 31/Mar/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: regression, transducers

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

 Description   

Error in build:

[java] ERROR in (seq-and-transducer) (TransformerIterator.java:86)
         [java] Uncaught exception, not in assertion.
         [java] expected: nil
         [java]   actual: java.lang.NullPointerException: null
         [java]  at clojure.lang.TransformerIterator.step (TransformerIterator.java:86)
         [java]     clojure.lang.TransformerIterator.hasNext (TransformerIterator.java:97)
         [java]     clojure.lang.RT.chunkIteratorSeq (RT.java:489)
         [java]     clojure.lang.RT.seqFrom (RT.java:518)
         [java]     clojure.lang.RT.seq (RT.java:509)
         [java]     clojure.core/seq (core.clj:135)
         [java]     clojure.core$print_sequential.invoke (core_print.clj:47)
         [java]     clojure.core/fn (core.clj:7362)
         [java]     clojure.lang.MultiFn.invoke (MultiFn.java:233)
         [java]     clojure.core$pr_on.invoke (core.clj:3549)
         [java]     clojure.core$pr.invoke (core.clj:3561)
         [java]     clojure.pprint$pprint_simple_default.invoke (dispatch.clj:144)

There is an error in the generative transducer eduction test that was added in CLJ-1669. This code in transducers.clj:

(apply eduction (into actions [coll]))

is not invoking eduction with actions and a collection. Rather it is putting the actions inside the collection and effectively using no transformation at all as in {{(eduction [1 2])}}, which always passes. The error seen is due to a bad function being passed - if actions happens to be nil (which I think is happening while shrinking another failure), something like (eduction (constantly nil) []) is being called, which we would not expect to work in the first place.

After fixing the bad eduction handling, I was only able to trigger failures with a high number of iterations and a very large number of transformations. The errors reported under these conditions are difficult to understand, I believe because they are hitting StackOverflow errors and the stack traces are being removed by the JVM.

I did some more investigation into whether we are actually generating useful generative tests and found that due to the large stack of transformations, virtually all tests were just producing exceptions, rather than more interesting behavior. I capped the number of transformations to 5 and saw much more useful and interesting tests being generated. I've also doubled the number of transducer tests being run in the patch and ran it locally with a much higher number with no failures.

Patch: clj-1684.patch






[CLJ-1683] test-reduce doesn't catch off-by-one error in IReduce Created: 25/Mar/15  Updated: 27/Mar/15  Resolved: 27/Mar/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: 1
Labels: None

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

 Description   

To implement the no-init arity of reduce, you have to use the first element of the sequence as an init value, and then you have to skip the first element when you iterate through the sequence. The tests in test-reduce, which focus on using reduce to sum up a bunch of numbers in a range, don't actually pin down this behavior, because the range used starts with zero, and an extra zero doesn't affect the sum.

Screened by: Alex Miller - this is a good tweak to get better tests for the hard this easy to mess up reduce behavior.






[CLJ-1681] reflection warning throws NPE for literal nil arg Created: 24/Mar/15  Updated: 27/Mar/15  Resolved: 27/Mar/15

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

Type: Defect Priority: Major
Reporter: Michael Blume Assignee: Michael Blume
Resolution: Completed Votes: 0
Labels: regression, typehints

Attachments: Text File clj-1681-v1.patch     Text File clj-1681-v2.patch     Text File clj-1681-v3.patch    
Patch: Code and Test
Approval: Ok

 Description   

Seen on another project, but reproducible with this:

user=> (set! *warn-on-reflection* true)
true
user=> (defn f [a] (.divide 1M a nil))
CompilerException java.lang.NullPointerException, compiling:(NO_SOURCE_PATH:2:13)
user=> (pst *e)
CompilerException java.lang.NullPointerException, compiling:(NO_SOURCE_PATH:21:13)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6740)
	clojure.lang.Compiler.analyze (Compiler.java:6524)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6721)
	clojure.lang.Compiler.analyze (Compiler.java:6524)
	clojure.lang.Compiler.analyze (Compiler.java:6485)
	clojure.lang.Compiler$BodyExpr$Parser.parse (Compiler.java:5861)
	clojure.lang.Compiler$FnMethod.parse (Compiler.java:5296)
	clojure.lang.Compiler$FnExpr.parse (Compiler.java:3925)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6731)
	clojure.lang.Compiler.analyze (Compiler.java:6524)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6721)
	clojure.lang.Compiler.analyze (Compiler.java:6524)
Caused by:
NullPointerException
	clojure.lang.Compiler.getTypeStringForArgs (Compiler.java:2440)
	clojure.lang.Compiler$InstanceMethodExpr.<init> (Compiler.java:1490)
	clojure.lang.Compiler$HostExpr$Parser.parse (Compiler.java:1000)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6733)
	clojure.lang.Compiler.analyze (Compiler.java:6524)

Cause: Regression in Clojure 1.6+ from patch for CLJ-1248. In printing the reflection warning message, if the argument's java class is null, an NPE results.

Approach: Check for null before getting class name.

Patch: clj-1681-v3.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 24/Mar/15 3:54 PM ]

Patch welcome.

Comment by Alex Miller [ 24/Mar/15 4:31 PM ]

Need test in the patch and having a simple way to repro in the ticket would be great.

Comment by Nicola Mometto [ 25/Mar/15 3:46 PM ]

here's a repro:

Clojure 1.7.0-master-SNAPSHOT
user=> (set! *warn-on-reflection* true)
true
user=> (defn f [a] (.divide 1M a nil))
CompilerException java.lang.NullPointerException, compiling:(NO_SOURCE_PATH:2:13)
Comment by Michael Blume [ 25/Mar/15 3:49 PM ]

Thanks! I was having some trouble with that.

Comment by Nicola Mometto [ 25/Mar/15 3:54 PM ]

Michael, me and Alex were discussing this ticket in IRC and Alex was proposing replacing the if expression with an implementation like:

(arg.hasJavaClass() && arg.getJavaClass() != null) ? arg.getJavaClass().getName() : "unknown"
Comment by Michael Blume [ 25/Mar/15 3:58 PM ]

Hm, that makes sense, I was thinking print "nil" instead of "unknown" but I guess the fact that the user is passing nil doesn't actually tell us the expected class of the argument (apart from that it's not a primitive)





[CLJ-1677] Add setLineNumber to LineNumberingPushbackReader Created: 17/Mar/15  Updated: 20/Mar/15  Resolved: 20/Mar/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: reader

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

 Description   

Add setLineNumber() to set line number on underlying LineNumberingReader.






[CLJ-1671] Clojure socket server Created: 09/Mar/15  Updated: 11/Sep/15  Resolved: 11/Sep/15

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

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

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

 Description   

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

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

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

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

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

where options are:

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

Run a repl client using telnet:

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

Patch: clj-1671-13.patch



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

New -4 patch changes:

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

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

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

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

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

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

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

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

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

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

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

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

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

Marking incomplete, pending at least the repl exit question.

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

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

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

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

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

OK thanks for the update.

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

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

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

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

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

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

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

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

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

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

Lifecycle concerns

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

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

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

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

More feedback forwarded from Stu:

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

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

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

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

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

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

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

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

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

Hey Laurent,

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

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

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

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

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

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

My first remark still stands, though:

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

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

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

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

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

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

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

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

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

Two comments on latest patch:

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

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

Comment by Stuart Halloway [ 04/Sep/15 1:41 PM ]

Major stuff:

  • I don't feel like we have nailed down the requirements for the session feature, so hard to comment on that
  • attach/detach metaphor for sessions feels weird, e.g. I cannot set a dynamic var inside there and have the other session see my change. We would instead just session state query and eval-in-session APIs.
  • something needs to happen if the session you are attached to goes away out from under you

Minor stuff:

  • socket backlog arg of 0 means "use default", is there some reason 50 is better?
  • I think 'Clojure' better than 'Socket' more disinguishing as thread names
  • catch-all exception eating scares me, couldn't stop-servers call the uncaught exception handler?
  • vec + map can be replaced by mapv
  • start-client seems a very odd name to me, as this is the server side client handler
Comment by Alex Miller [ 04/Sep/15 4:45 PM ]

Added new -12 patch with the following changes per Stu's comments:

  • In the scenario where an attached session goes away, you now lose that attachment and get an exception tell you so the next time you try to eval an expression.
  • Changed socket backlog to 0 (50 is the default, but I wasn't aware that 0 would get the default)
  • Changed thread names
  • Changed stop-server to shutdown each server in a future independently - exceptions will bubble up to default uncaught exception handler
  • Replaced vec+map with mapv
  • Changed start-client to accept-connection

Deferred discussion of session intent.

Comment by Alex Miller [ 09/Sep/15 9:26 AM ]

The -13 patch strips out the session attach/detach/list functionality. Rescued description of those features in -12 here:

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

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

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

Comment by Alex Miller [ 09/Sep/15 9:42 AM ]

Add socket-change.patch for change log update.





[CLJ-1669] Move LazyTransformer to an iterator strategy, extend eduction capabilities Created: 04/Mar/15  Updated: 19/May/15  Resolved: 19/May/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: Alex Miller
Resolution: Completed Votes: 0
Labels: transducers

Attachments: Text File clj-1669-2.patch     Text File clj-1669-3.patch     Text File clj-1669-4.patch     Text File clj-1669-5.patch     Text File clj-1669-6.patch     Text File clj-1669.patch    
Patch: Code and Test
Approval: Ok

 Description   
  • LazyTransformer does a lot of work to be a seq. Instead, switch to creating a transforming iterator.
  • Change sequence to wrap iterator-seq around the transforming iterator.
  • Change the iterator-seq implementation to be chunked. IteratorSeq will no longer be used but is left in case of regressions for now.
  • Change Eduction to provide iteration directly via the transforming iterator.
  • Extend eduction to support multiple xforms.

Performance:

Compared:

  • alpha5 == before any change
  • alpha6 == after clj-1669-6.patch was applied
  • beta3 == latest, includes range enhancement, expanding mapcat enhancement, etc

;; using java 1.8
(use 'criterium.core)
(def s (range 1000))
(def v (vec s))
(def s50 (range 50))
(def v50 (vec s50))

expr alpha5 s alpha5 v alpha6 s alpha6 v beta3 s beta3 v
non-chunking transform            
(into [] (->> s (interpose 5) (partition-all 2))) 432 us 437 us 413 us 411 us 353 us 414 us
(into [] (->> s (eduction (interpose 5) (partition-all 2)))) * 117 us 118 us 117 us 113 us 116 us 113 us
1 chunking transform            
(into [] (map inc s)) 43 us 45 us 35 us 31 us 32 us 36 us
(into [] (map inc) s) 19 us 19 us 18 us 18 us 18 us 16 us
(into [] (sequence (map inc) s)) 100 us 54 us 97 us 65 us 66 us 64 us
(into [] (eduction (map inc) s)) 24 us 19 us 24 us 20 us 20 us 19 us
(doall (map inc (eduction (map inc) s))) 219 us 203 us 98 us 78 us 93 us 89 us
2 chunking transforms        
(into [] (map inc (map inc s))) 53 us 56 us 53 us 54 us 61 us 58 us
(into [] (comp (map inc) (map inc)) s) 13 us 26 us 30 us 26 us 31 us 31 us
(into [] (sequence (comp (map inc) (map inc)) s)) 111 us 64 us 98 us 73 us 83 us 80 us
(into [] (eduction (map inc) (map inc) s)) * 58 us 31 us 58 us 31 us 30 us 31 us
(doall (map inc (eduction (map inc) (map inc) s))) * 240 us 212 us 114 us 93 us 105 us 102 us
expand transform            
(into [] (mapcat range (map inc s50))) 74 us 73 us 67 us 68 us 37 us 39 us
(into [] (sequence (comp (map inc) (mapcat range)) s50)) 111 us 102 us 166 us 159 us 99 us 98 us
(into [] (eduction (map inc) (mapcat range) s50)) * 65 us 64 us 57 us 56 us 27 us 27 us
materialized eduction            
(sort (eduction (map inc) s)) ERR ERR 99 us 77 us 77 us 77 us
(->> s (filter odd?) (map str) (sort-by last)) 1.10 ms 1.25 ms 1.15 ms 1.19 ms 1.14 ms 1.13 ms
(->> s (eduction (filter odd?) (map str)) (sort-by last)) ERR ERR 1.18 ms 1.15 ms 1.13 ms 1.13 ms
  • used comp to combine xforms as eduction only took one in the before case

Patch: clj-1669-6.patch

Screened by:



 Comments   
Comment by Michael Blume [ 05/Mar/15 3:52 PM ]

Nice, I like the direction on this.

CLJ-1515 currently breaks this patch (LongRange cannot be converted to Iterable), but I imagine that'll get better when it absorbs the changes from CLJ-1603

Comment by Alex Miller [ 06/Mar/15 8:11 AM ]

Yeah. colls should be mapped through RT.iter() to catch more cases.

Comment by Alex Miller [ 06/Mar/15 9:52 AM ]

To do:

  • remove Seqable from Eduction
  • support Iterable in RT.toArray()
  • more eduction pipeline tests that require realization at end
Comment by Alex Miller [ 06/Mar/15 1:00 PM ]

Perf numbers show pretty worse results from sequence, will dig in further.

Comment by Alex Miller [ 13/Mar/15 7:41 AM ]

For the s timings, we've actually introduced more steps into the stack:

OLD reduce with s:

LazyTransformer
   seq (range) - every transformation is another layer here

NEW reduce with s:

IteratorSeq 
  TransformingIterator (handles N transformations in 1 step)
    SeqIterator
      seq (range)
Comment by Alex Miller [ 20/Mar/15 10:08 AM ]

Look at perf for:

  • ->> eduction transformation
  • transformation comparison that doesn't support chunking
  • more into vector iteration case
Comment by Alex Miller [ 21/Mar/15 8:45 AM ]

The -5 patch is same -3 except all uses of IteratorSeq have been replaced with a ChunkedCons that is effectively a chunked version of the old IteratorSeq. While no one calls it, I left IteratorSeq in the code base in case of regression.

Generally, the chunked iterator seq reduces the cost in a number of the worst cases and also is a clear benefit in making seqs over a result of eduction or sequence faster to traverse (as they are now chunked).

I think the one potential issue is that seqs over iterators are now chunked when they were not before which could change programs that expect their stateful iterator to be traversed one at a time. This change could be isolated to just to sequence and seq-iterator and mitigated by not changing RT.seqFrom() and seq-iterator to use the new chunking behavior only in sequence and/or with a new chunked-iterator-seq to make it more explicit. The sequence over xf is new so no possible regression there, everything else would just be opt-in.

Comment by Rich Hickey [ 27/Mar/15 9:49 AM ]

push as is but leave unresolved, for perf tweaks

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

clj-1669-6 is identical to clj-1669-5 but removes two commented out debugging lines that were inadvertently included.

Comment by Alex Miller [ 18/May/15 9:47 AM ]

updated for beta3 numbers





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

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

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: io, test

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

 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-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-1659] compile leaks files Created: 16/Feb/15  Updated: 10/Aug/15  Resolved: 17/Jul/15

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

Type: Defect Priority: Major
Reporter: Ralf Schmitt Assignee: Unassigned
Resolution: Completed Votes: 3
Labels: compiler, ft

Attachments: Text File clj-1659.patch     Text File clj-1659-v2.patch     Text File clj-1659-v3.patch    
Patch: Code
Approval: Ok

 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)
    ...

Cause: getLastModified() opens the URLConnection's InputStream but does not close it.

Approach: 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.

Patch: clj-1659-v3.patch

Screened by: Alex Miller

[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



 Comments   
Comment by Pietro Menna [ 06/May/15 11:10 AM ]

First attempt Patch

Comment by Pietro Menna [ 06/May/15 11:13 AM ]

Hi Alex,

This is my first patch to Clojure and to any OSS. So maybe I will need a little guidance. I follow the steps on how to generate the patch and just uploaded the patch to this thread.

The link from Stack Overflow was good, but unfortunately it is not possible to cast to HttpURLConnection in order to have the .disconnect() method.

Please, let me know if I should attempt anything else.

Kind regards,

Pietro

Comment by Andy Fingerhut [ 06/May/15 11:20 AM ]

Seems that creating a test for this to be run on every build might be difficult.

Have you verified that on Windows it has the desired effect?

Comment by Ralf Schmitt [ 06/May/15 4:49 PM ]

I don't understand how the patch solves that issue. It just sets connection to null. Or am I missing something?

You can test for the file leak with the following program. This works on windows and Linux for me.

leak-test.clj
;; test file leak
;; on UNIX-like systems set hard limit on open files via
;; ulimit -H -n 200 and then run
;; java -jar clojure-1.6.0.jar leak-test.clj

(let [file (java.io.File. "test-leak.txt")
      url (.. file toURI toURL)]
  (doseq [x (range 2000)]
    (print x " ")
    (flush)
    (spit file "")
    (clojure.lang.RT/lastModified url nil)
    (assert (.delete file) "delete failed")))
Comment by Ralf Schmitt [ 06/May/15 5:21 PM ]

dammit, the formatting is wrong. but this patch seems to fix the problem for me (tested on linux).

Comment by Ralf Schmitt [ 06/May/15 6:16 PM ]

indentation fixed

Comment by Andy Fingerhut [ 07/May/15 8:45 AM ]

Ralf, thanks for the patch. I can't say if or when this ticket will be considered for a change to Clojure, but I do know that patches are only considered if they were written by someone who has signed a CA. Were you considering doing so? You can do it on-line here if you wish: http://clojure.org/contributing

Also, patches should be in a slightly different format that include the author's name, email, date of change, etc. Instructions for creating a patch in that format are here: http://dev.clojure.org/display/community/Developing+Patches

Comment by Ralf Schmitt [ 07/May/15 8:50 AM ]

Thanks for the explanation. I've signed the CA and I will update the patch.

Comment by Ralf Schmitt [ 07/May/15 9:34 AM ]

patch vs current master, created with git format-patch

Comment by Andy Fingerhut [ 26/May/15 8:41 AM ]

Ralph, it would be good if all attached files on a ticket have different names, for clarity when referring to them. Could you remove your clj-1659.patch and upload it with a different name, e.g. clj-1659-v2.patch ?

Comment by Ralf Schmitt [ 26/May/15 8:49 AM ]

upload my last patch with non-conflicting filename

Comment by Ralf Schmitt [ 26/May/15 8:52 AM ]

yes, sorry about that. I don't know how to remove my previous patches. (ok, found it now)

Comment by Sven Richter [ 08/Jun/15 2:57 AM ]

This one is keeping me from using boot on windows and seeing how far the 1.7 release process is I would like to express my strong wish that this one makes it into the 1.7 release.

Thanks everyone for their hard work

Comment by Alex Miller [ 18/Jun/15 4:09 PM ]

The clj-1659-v3.patch is same as v2, just removed unneeded braces to match rest of compiler.

Comment by Erik Dannenberg [ 06/Aug/15 12:09 PM ]

Issue still persists with Clojure 1.8.0-alpha4 and latest boot. Running Windows 8.1.

Example app that should reproduce the error: https://github.com/danielsz/sente-boot

clone, run boot dev, change a source file => file exists errors

Any chances to get a fix for this into Clojure 1.7? It's pretty much a show stopper for us as it prevents the reloaded workflow for our windows devs.

Comment by Ralf Schmitt [ 06/Aug/15 4:21 PM ]

This must be another issue. Please try to find the leak with http://file-leak-detector.kohsuke.org/

(I once dug into a similar issue in boot's bug tracker and the problem was somewhere in the cljs compiler).

Comment by Erik Dannenberg [ 10/Aug/15 11:46 AM ]

I attached the file leak detector and ran boot with clojure 1.7 and 1.8-alpha4, the stacktraces for both are a bit different but from what i can tell the leak cause is still the same and looking very similiar to the stacktrace posted in the issue description. I do however see the cljs compiler further down in the stacktrace.

Should i open an issue on CLJS tracker?

Comment by Ralf Schmitt [ 10/Aug/15 11:51 AM ]

This is hard to tell without a stacktrace...

Comment by Erik Dannenberg [ 10/Aug/15 11:59 AM ]
1 descriptors are open
#1 C:\Users\ed\.boot\cache\tmp\Users\ed\projekte\sente-system\5ho\-c4hx1l\adzerk\boot_reload.cljs by thread:clojure-agent-send-off-pool-0 on Mon Aug 10 17:54:39 CEST 2015
	at java.io.FileInputStream.<init>(FileInputStream.java:139)
	at java.io.FileInputStream.<init>(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 cljs.util$last_modified.invokeStatic(util.cljc:136)
	at cljs.util$last_modified.invoke(util.cljc)
	at cljs.analyzer$requires_analysis_QMARK_.invokeStatic(analyzer.cljc:2179)
	at cljs.analyzer$requires_analysis_QMARK_.invoke(analyzer.cljc)
	at cljs.analyzer$requires_analysis_QMARK_.invokeStatic(analyzer.cljc:2164)
	at cljs.analyzer$requires_analysis_QMARK_.invoke(analyzer.cljc)
	at cljs.analyzer$analyze_file.invokeStatic(analyzer.cljc:2220)
	at cljs.analyzer$analyze_file.invoke(analyzer.cljc)
	at cljs.analyzer$analyze_deps.invokeStatic(analyzer.cljc:1291)
	at cljs.analyzer$analyze_deps.invoke(analyzer.cljc)
	at cljs.analyzer$eval1868$fn__1870.invoke(analyzer.cljc:1545)
	at clojure.lang.MultiFn.invoke(MultiFn.java:251)
	at cljs.analyzer$analyze_seq.invokeStatic(analyzer.cljc:1900)
	at cljs.analyzer$analyze_seq.invoke(analyzer.cljc)
	at cljs.analyzer$analyze$fn__2118.invoke(analyzer.cljc:1992)
	at cljs.analyzer$analyze.invokeStatic(analyzer.cljc:1985)
	at cljs.analyzer$analyze.invoke(analyzer.cljc)
	at cljs.compiler$compile_file_STAR_$fn__3216.invoke(compiler.cljc:1027)
	at cljs.compiler$with_core_cljs.invokeStatic(compiler.cljc:968)
	at cljs.compiler$with_core_cljs.invoke(compiler.cljc)
	at cljs.compiler$compile_file_STAR_.invokeStatic(compiler.cljc:988)
	at cljs.compiler$compile_file_STAR_.invoke(compiler.cljc)
	at cljs.compiler$compile_file$fn__3248.invoke(compiler.cljc:1129)
	at cljs.compiler$compile_file.invokeStatic(compiler.cljc:1109)
	at cljs.compiler$compile_file.invoke(compiler.cljc)
	at cljs.compiler$compile_root.invokeStatic(compiler.cljc:1181)
	at cljs.compiler$compile_root.invoke(compiler.cljc)
	at cljs.closure$compile_dir.invokeStatic(closure.clj:385)
	at cljs.closure$compile_dir.invoke(closure.clj)
	at cljs.closure$eval3614$fn__3615.invoke(closure.clj:425)
	at cljs.closure$eval3567$fn__3568$G__3558__3575.invoke(closure.clj:331)
	at cljs.closure$eval3627$fn__3628.invoke(closure.clj:439)
	at cljs.closure$eval3567$fn__3568$G__3558__3575.invoke(closure.clj:331)
	at adzerk.boot_cljs.impl.CljsSourcePaths$fn__4011.invoke(impl.clj:14)
Comment by Alex Miller [ 10/Aug/15 12:30 PM ]

Looks like similar problem in CLJS to me - I would open a ticket there.

Comment by Erik Dannenberg [ 10/Aug/15 1:35 PM ]

Done. CLJS-1416





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

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

Type: Defect Priority: Major
Reporter: Alexander Yakushev Assignee: Stuart Halloway
Resolution: Completed Votes: 1
Labels: None
Environment:

Everywhere, but so far relevant only on Android 5.0


Attachments: File CLJ-1657-patch.diff    
Patch: Code
Approval: Ok

 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



 Comments   
Comment by Alexander Yakushev [ 18/Mar/15 5:31 AM ]

I attached a patch that resolves the issue. The change makes `generate-proxy` treat abstract methods like interface methods. Which means, if the implementation for the method is not provided, it will throw unsupported exception rather than try to call the parent method (which doesn't exist).

Comment by Michael Blume [ 18/Mar/15 12:50 PM ]

Alexander: Awesome, thanks =)

Note: If you use git format-patch after making a commit, you can generate a patch file with your name/e-mail and a commit message that a clojure maintainer can apply directly to clojure as a new commit.

Comment by Alex Miller [ 18/Mar/15 12:53 PM ]

The patch process is documented here: http://dev.clojure.org/display/community/Developing+Patches

Comment by Alexander Yakushev [ 18/Mar/15 4:38 PM ]

Sorry, I should have checked the guidelines first. I uploaded a new patch, hope it is correct now.

Comment by Alexander Yakushev [ 18/Jul/15 10:59 AM ]

Anything that holds this back? Now that Clojure is in volatile after-release state it's a good time to deal with it.

Comment by Alex Miller [ 18/Jul/15 12:25 PM ]

No, wasn't in triaged but is now.





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

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

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

Attachments: Text File clj-1653-2.patch     Text File clj-1653-3.patch     Text File CLJ-1653-toString-for-EmptyList.patch    
Patch: Code and Test
Approval: Ok

 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 () {} [])
"() {} []"

Patch: clj-1653-3.patch

Screened by: Alex Miller



 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.

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

Added -2 patch just to fix trailing whitespace warnings, identical otherwise.

Comment by Alex Miller [ 09/Oct/15 9:13 AM ]

Updated -3 patch to apply to master, no semantic changes, attribution retained.





[CLJ-1648] Use equals() instead of == when resolving Symbol Created: 22/Jan/15  Updated: 12/May/15  Resolved: 12/May/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: Steven Yi Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: Compiler

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

 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 ;; reported from System.out.println code I put into Compiler.java for == vs .equals()
nil

The result is that a new Symbol is allocated, when the previous one should be returned.

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

Patch: resolve-symbol-equals.diff

Screened by: Alex Miller

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






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

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

Type: Defect Priority: Minor
Reporter: Fabio Tudone Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: compiler, ft, 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     Text File CLJ-1645-protocol-class-has-no-source-file-information-w-repl.patch    
Patch: Code and Test
Approval: Ok

 Description   

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

Approach: In gen-interface, if the *source-path* indicates this is not a REPL-generated interface, invoke the proper ASM method to set the source file. Per JVM expectations, this is the actual file name, not the file path.

Patch: CLJ-1645-protocol-class-has-no-source-file-information-w-repl.patch

Screened by: Alex Miller



 Comments   
Comment by Fabio Tudone [ 22/Jun/15 11:45 AM ]

Any chances this will get into Clojure 1.7.0?

Comment by Alex Miller [ 22/Jun/15 12:28 PM ]

No, sorry.

Comment by Alex Miller [ 22/Jun/15 1:10 PM ]

It looks like the patch does not handle the repl case where the file will be "NO_SOURCE_FILE". Can you add a (when (not= "NO_SOURCE_FILE" source-path) ...) check around calling the visitSource invocation?

Comment by Fabio Tudone [ 22/Jun/15 3:16 PM ]

As requested

Comment by Alex Miller [ 22/Jun/15 3:57 PM ]

My last comment formatted my "earmuff stars" around source-path and you included that in the patch, which doesn't compile. Should be *source-path*, not source-path in the condition.

Comment by Fabio Tudone [ 22/Jun/15 4:02 PM ]

Right, re-attaching.

Comment by Alex Miller [ 22/Jun/15 4:18 PM ]

Fabio, this looks good and I would like to move it forward, but I just realized we don't have a signed Contributor Agreement from you. If you could do so, that would be great - the information and electronic form can be found at http://clojure.org/contributing .

Comment by Fabio Tudone [ 22/Jun/15 4:24 PM ]

Sure, with pleasure. Please also consider that the original patch is not mine though, but Yanxiang Lou's.

Comment by Fabio Tudone [ 22/Jun/15 4:27 PM ]

Signed.





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

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

Type: Defect Priority: Minor
Reporter: Michael Blume Assignee: Michael Blume
Resolution: Completed Votes: 0
Labels: arrays, ft

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

 Description   

The into-array doc string implies that into-array will fall back to an Object array if aseq is supplied, but if the first element of aseq is nil, an NPE occurs.

user=> (doc into-array)
-------------------------
clojure.core/into-array
([aseq] [type aseq])
  Returns an array with components set to the values in aseq. The array's
  component type is type if provided, or the type of the first value in
  aseq if present, or Object. All values in aseq must be compatible with
  the component type. Class objects for the primitive types can be obtained
  using, e.g., Integer/TYPE.
user=> (into-array [nil 1 2])
NullPointerException   clojure.lang.RT.seqToTypedArray (RT.java:1691)

Approach: Check for nil and use Object as the array type.

Patch: CLJ-1644-array-first-nil-v2.patch

Screened by: Alex Miller



 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)

Comment by Phill Wolf [ 12/Mar/15 7:06 PM ]

Searching through the sequence for a non-null item is not consistent with into-array's docstring. The docstring says, "The array's component type is type if provided, or the type of the first value in aseq if present, or Object." In keeping with the docstring, wouldn't a null first item suggest an array of Object?

Working harder than that (by searching the sequence) only delays the inevitable: a whole sequence of nulls producing an Object array, instead of an array of the type the programmer expected, and triggering a run-time crash.

In summary: this patch goes farther than necessary, but even so, it does not cure the risk of unexpected results from nulls. A simpler remedy – returning an array of Object if the first item is null – would be consistent with the docstring and avoid raising unfounded expectations. Adding a statement that null is of type Object to the docstring could help programmers avoid falling into the trap.

Comment by Alex Miller [ 12/Mar/15 8:29 PM ]

No search through the sequence will pass screening, please just add a nil check.

Comment by Michael Blume [ 13/Mar/15 4:39 PM ]

done





[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-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-1638] Regression - PersistentVector.create(List) was removed in 1.7.0-alpha5 Created: 12/Jan/15  Updated: 20/Mar/15  Resolved: 20/Mar/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: Unassigned
Resolution: Completed 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: Ok

 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-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-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-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