<< Back to previous view

[CLJ-1933] please add unless macro for symmetry with when Created: 27/May/16  Updated: 27/May/16  Resolved: 27/May/16

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

Type: Enhancement Priority: Trivial
Reporter: Ernesto Alfonso Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: enhancement


 Description   

Is there a reason there is a `when` macro but no `unless`? I think it useful, CL uses it, adds consistency/symmetry and conciseness to code.

(defmacro unless [test & body]
`(when (not ~test) ~@body))



 Comments   
Comment by Ragnar Dahlen [ 27/May/16 2:28 PM ]

There is already when-not: http://clojure.github.io/clojure/clojure.core-api.html#clojure.core/when-not

Comment by Alex Miller [ 27/May/16 3:47 PM ]

As Ragnar says, when-not is equivalent.





[CLJ-1932] Add clojure.spec/explain-str to return explain output as a string Created: 25/May/16  Updated: 26/May/16  Resolved: 26/May/16

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

Type: Enhancement Priority: Major
Reporter: D. Theisen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: spec

Approval: Vetted

 Description   

Currently explain prints to *out* - add a function explain-str that returns the explain output as a string.



 Comments   
Comment by Alex Miller [ 25/May/16 9:51 AM ]

You can easily capture the string with (with-out-str (s/explain spec data)).

Comment by Alex Miller [ 26/May/16 8:35 AM ]

explain-str was added in https://github.com/clojure/clojure/commit/575b0216fc016b481e49549b747de5988f9b455c for 1.9.0-alpha3.





[CLJ-1931] clojure.spec/with-gen throws AbstractMethodError Created: 24/May/16  Updated: 25/May/16  Resolved: 25/May/16

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

Type: Defect Priority: Major
Reporter: Tyler van Hensbergen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

OSX Yosemite 10.10.5



 Description   

An AbstractMethodError is encountered when trying to evaluate a s/def form with the generator-fn overridden using s/with-gen.

(ns spec-fun.core
  (:require [clojure.spec :as s]
            [clojure.test.check.generators :as gen]))

(s/def ::int integer?)

(s/def ::int-vec
  (s/with-gen
    (s/& (s/cat :first ::int
                :rest  (s/* integer?)
                :last  ::int)
         #(= (:first %) (:last %)))
    #(gen/let [first (s/gen integer?)
               rest  (gen/such-that
                      (partial at-least 3)
                      (gen/vector (s/gen integer?)))]
       (concat [first] rest [first]))))
;;=> AbstractMethodError

;; The generator works independently
(gen/generate (gen/let [first (s/gen integer?)
                        rest  (gen/such-that
                               (partial at-least 3)
                               (gen/vector (s/gen integer?)))]
                (concat [first] rest [first])))
;;=> (-13 8593 -33421108 4 6697 0 35835865 -94366552 1 14165115 -4090 42 775 -15238320 233500020 -1 -13)

;; And so does the spec:
(s/def ::int-vec
  (s/& (s/cat :first ::int
              :rest  (s/* integer?)
              :last  ::int)
       #(= (:first %) (:last %))))

(s/conform ::int-vec '(-13 8593 -33421108 4 6697 0 35835865 -94366552 1 14165115 -4090 42 775 -15238320 233500020 -1 -13))
;;=> {:first -13, :rest [8593 -33421108 4 6697 0 35835865 -94366552 1 14165115 -4090 42 775 -15238320 233500020 -1], :last -13}


 Comments   
Comment by Alex Miller [ 25/May/16 10:13 AM ]

Fixed in commit https://github.com/clojure/clojure/commit/ec2512edad9c0c4a006980eedd2a6ee8679d4b5d for 1.9 alpha2.





[CLJ-1930] IntelliJ doesn't allow debugging of clojure varargs from Java Created: 22/May/16  Updated: 22/May/16  Resolved: 22/May/16

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

Type: Defect Priority: Critical
Reporter: Mathias Bogaert Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: PNG File intellij.png    

 Description   

When trying to debug evaluate Datomic's datoms API IntelliJ or the method thows "java.lang.IllegalArgumentException : Invalid argument count: expected 2, received 3". Debugging Java varargs is not an issue.

Using IntelliJ 2016.2 CE.



 Comments   
Comment by Mathias Bogaert [ 22/May/16 9:06 AM ]

Datomic 0.9.5359, JDK 1.8.0_74, OS/X 10.11.5.

Comment by Kevin Downey [ 22/May/16 1:56 PM ]

hi, this is the issue tracker for the Clojure programming language, not Datomic or Intellij. http://www.datomic.com/support.html lists various support options for datomic

Comment by Alex Miller [ 22/May/16 3:55 PM ]

Agreed with Kevin, this is an issue with Cursive and you can find that tracker here:

https://github.com/cursive-ide/cursive/issues

I think this existing ticket is relevant:

https://github.com/cursive-ide/cursive/issues/326





[CLJ-1909] Using thrown? in exceptions fails without doall Created: 02/Apr/16  Updated: 02/Apr/16  Resolved: 02/Apr/16

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

Type: Defect Priority: Major
Reporter: Shriphani Palakodety Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

OS: OS X and testing using lein test



 Description   

I have added a small example in this repo: https://github.com/shriphani/thrown-test

See the test in https://github.com/shriphani/thrown-test/blob/master/test/thrown_test/core_test.clj

The first assertion fails, the second passes.

The output I get is: https://gist.github.com/shriphani/d9351d062f2f5c211879ef87c13277ac



 Comments   
Comment by Alex Miller [ 02/Apr/16 10:02 AM ]

In the example without doall, map will return a lazy seq that is not realized and thus you never encounter the exception. This is the expected behavior so I am declining the ticket.





[CLJ-1904] clojure.template/apply-template - 'unsupported binding form' when re-binding the input symbols Created: 29/Mar/16  Updated: 29/Mar/16  Resolved: 29/Mar/16

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

Type: Defect Priority: Minor
Reporter: James Henderson Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   
(clojure.template/apply-template '[s]
                                 '(let [s "foo"]
                                    s)
                                 ["s"])

returns

(let ["s" "foo"] 
  "s")

which then fails with Unsupported binding form: s - whereas it seems that it shouldn't replace the binding symbol in this case?

This came up when using clojure.test, as follows:

(t/are [req resp] (= resp
                     (let [handler (-> (fn [{:keys [uri] :as req}]
                                         {:body (str "You requested: " uri)})
                                       middleware-under-test)]

                       (handler req)))
  {:uri "..."} {:status 200, :body "..."})

macro-expands to

(do
  (t/is
   (=
    {:status 200, :body "..."}
    (let [handler (-> (fn [{:keys [uri], :as {:uri "..."}}]
                        {:body (str "You requested: " uri)})
                      middleware-under-test)]
      (handler {:uri "..."})))))

which, in this case, then threw Bad binding form, expected symbol, got: {:uri "..."}.

An easy work-around is to rename/remove the req parameter in the expr, although this seems like it should be a valid use-case?



 Comments   
Comment by Alex Miller [ 29/Mar/16 6:59 AM ]

It seems to me like the problem here is in 'are', not in apply-template, which is just blindly doing what it's been told to do.

Comment by James Henderson [ 29/Mar/16 8:09 AM ]

Sure, the docstring says that it blindly replaces symbols - but doesn't that mean that all of the callers of apply-template/do-template have to take this issue into account? If so, would it be better to fix it here?

If not, no worries - would you like me to file an issue against clojure.test?

Comment by Alex Miller [ 29/Mar/16 9:59 AM ]

I do not think it is reasonable for a generic utility like apply-template to any special-case thing here (esp when the set of cases is open-ended and hard/impossible to detect).

clojure.test/are points to clojure.template for understanding what will be done and apply-template says "will recursively replace argument symbols in expr with their corresponding values". I think what you are seeing is the expected behavior. That is, there are limits to what are templates do and you have exceeded them. The workaround seems pretty simple.

I'm going to decline this as I don't see anything reasonable that needs to change.





[CLJ-1902] Remove overhead of if-not Created: 16/Mar/16  Updated: 16/Mar/16  Resolved: 16/Mar/16

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

Type: Enhancement Priority: Trivial
Reporter: Jeroen van Dijk Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: enhancement

Attachments: Text File clj_1902.patch    

 Description   

The `(if-not x a b)` macro expands to `(if (not x) a b`, but it could be more efficient by just expanding to `(if x b a)`

Why is this important? I've always found it more readable to have the biggest condition to be placed second. This allows you to see the different paths easier.

So one would change:

(if x
  (let [...] 
    .
    .
    .
    a)
  b)

To

(if-not x
  b
  (let [...] 
    .
    .
    .
    a))

I think you would agree that the second one is more readable. However currently with `if-not` there is always the tiny performance counter-argument to not doing this.



 Comments   
Comment by Jeroen van Dijk [ 16/Mar/16 5:58 AM ]

The patch doesn't include any new tests as breaking `if-not` already broke the "compile-clojure" tests.

Comment by Alex Miller [ 16/Mar/16 8:11 AM ]

Why do you think there's a performance difference?

Comment by Alex Miller [ 16/Mar/16 8:23 AM ]

Quick benchmark shows about < 1 ns difference.

vecperf.bench=> (bench (if (odd? 1) 1 2))
Evaluation count : 10214445780 in 60 samples of 170240763 calls.
             Execution time mean : 4.215496 ns
    Execution time std-deviation : 0.025472 ns
   Execution time lower quantile : 4.179194 ns ( 2.5%)
   Execution time upper quantile : 4.272295 ns (97.5%)
                   Overhead used : 1.667409 ns
nil
vecperf.bench=> (bench (if (not (odd? 1)) 2 1))
Evaluation count : 9389004780 in 60 samples of 156483413 calls.
             Execution time mean : 4.768709 ns
    Execution time std-deviation : 0.028476 ns
   Execution time lower quantile : 4.721174 ns ( 2.5%)
   Execution time upper quantile : 4.824708 ns (97.5%)
                   Overhead used : 1.667409 ns
Comment by Jeroen van Dijk [ 16/Mar/16 8:47 AM ]

Yeah sounds I'm a bit pedantic if you put it that way. The benchmark is indeed not very convincing. I'm ok if you close this issue.





[CLJ-1894] Include namespace when stringifying keys in maps Created: 22/Feb/16  Updated: 24/Feb/16  Resolved: 24/Feb/16

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

Type: Enhancement Priority: Major
Reporter: Rafal Szalanski Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: walk
Environment:

OS X, Java 8, Clojure 1.8


Attachments: Text File full-name.patch    
Patch: Code and Test

 Description   

I noticed that if one wants to stringify keys in map using clojure.walk/stringify-keys and the keywords or symbols contain namespaces, for example:

(clojure.walk/stringify-keys {:a 1 :b/c 2})

then the result will be equal to {"a" 1 "c" 2}. The docstring for clojure.walk/stringify-keys says:

Recursively transforms all map keys from keywords to strings.

which to my mind implies the namespace should be included in the result map so that reverse operation clojure.walk/keywordize-keys can re-create the initial map.

The patch I am proposing adds a function full-name to clojure.core namespace which returns full string representation of a keyword including the namespace (if it's present). I also modify clojure.walk/stringify-keys to use that function instead of clojure.core/name.

The change should be 100% compatible with any Clojure code out there. I am making an assumption that people who came up against this problem found a different way of solving that problem, even re-design everything to use keywords instead of strings. Keywords are one of the most commonly used parts of the language and have clear benefits over strings (i.e. they are functions).



 Comments   
Comment by Alex Miller [ 24/Feb/16 1:53 PM ]

I disagree with your assumption re current use - it's possible that the current behavior is desired (or at least relied-upon) by some existing caller. I'm not willing to change this default behavior.

Instead, I would note that stringify-keys and keywordize-keys are the same function with a different transformation. It would be better to extract that more generic function (transform-keys?), refactor stringify-keys and keywordize-keys in terms of it, and let users supply any transformation function they want.

I'm going to decline this ticket as is, as will not make the suggested change. The other idea would be a reasonable alternative ticket. (Although it does risk running into an expansion of purpose to include shallow/deep alternative and value transformations - all things I think are valuable and in common use).





[CLJ-1893] Clojure returns nil as the empty java.util.Map$Entry Created: 13/Feb/16  Updated: 15/Feb/16  Resolved: 13/Feb/16

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

Type: Enhancement Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

1.8



 Description   
(empty (first {1 2}))
;; => nil

empty of a map entry should return the empty vector (as in clojurescript), because then the zipper for edn becomes very elegant:

(def coll-zipper (partial zip/zipper coll? seq #(into (empty %1) %2)))


 Comments   
Comment by Alex Miller [ 13/Feb/16 11:11 PM ]

A map entry is considered to be a 2-tuple of key and value (the particular MapEntry class should be considered an implementation detail), where tuple implies ordered and indexed access like a vector, but also a fixed size. The docstring for empty says "Returns an empty collection of the same category as coll, or nil". To me, having a map entry return a vector violates the "same category" language, although because map entry shares many aspects with vectors this is admittedly open to interpretation.

Overall, I think returning nil is more consistent with the ideals behind map entries and empty. Similar arguments were applied to records (as they have known fields) and that's why empty does not work on records. I concede there is some utility to having map entries empty to a vector.

However, I suspect any of these decisions are more likely to shake out in some future when tuples are reconsidered and the MapEntry classes are replaced with tuples. Because of that, I don't think this ticket is going anywhere now and I'm going to decline it.

Comment by Nicola Mometto [ 15/Feb/16 11:12 AM ]

This feels really counter-intuitive given that

Comment by Nicola Mometto [ 15/Feb/16 11:13 AM ]

This feels really counter-intuitive given that

(vector? (first {:a 1}))
returns true and `empty?` on vectors is supposed to return the empty vector





[CLJ-1892] Subtraction floating point numbers error Created: 12/Feb/16  Updated: 12/Feb/16  Resolved: 12/Feb/16

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

Type: Defect Priority: Major
Reporter: Umarov German Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: PNG File floating bug.png    

 Description   

Subtraction of floating point numbers gives wrong result.
code:
(- 12.1 42.9)
result:
-30.799999999999997

JRE 1.7 patch 25/1.8 patch 72



 Comments   
Comment by Alex Miller [ 12/Feb/16 9:49 AM ]

By default, Clojure floating point numbers are Java doubles internally. Java doubles are IEEE 754 64-bit floating point numbers. Not all floating points can be represented exactly in this format (because you can't squeeze an infinite number of floats into a finite number of bits). Results like this are typical and expected - http://docs.oracle.com/cd/E19957-01/806-3568/ncg_goldberg.html.

In Clojure, you can get arbitrary precision floating point (Java BigDecimal) by appending an M to the number:

user=> (- 12.1M 42.9M)
-30.8M




[CLJ-1883] references to a symbol ending with ? that came from a foreign namespace confuses the compiler inside a :cljc block Created: 13/Jan/16  Updated: 13/Jan/16  Resolved: 13/Jan/16

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

Type: Defect Priority: Major
Reporter: Geraldo Lopes de Souza Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: File bug.cljc     File bug.tar.bz2    

 Description   

I've found a situation where I'm requiring a om.tempid on a .cljc namespace. The require is
[om.tempid :as tempid :refer [tempid?]]
The reference of tempid? on line 24 of bug.cljc generates
java.lang.IllegalArgumentException: Unable to resolve classname: clojure.core$boolean@7521bffd, compiling/home/geraldo/bug/src/bug/bug.cljc:14:5)

To circumvent I've created a local alias of the tempid? function (line 26). It seems that the ? at the end of the name is causing the trouble because if I reference om.tempid/tempid it does not trigger. Note that the local alias was ended with ? to show that only when is a foreign ns that the problem is present.

Regards and forgive my english.

Geraldo Lopes de Souza



 Comments   
Comment by Alex Miller [ 13/Jan/16 8:20 AM ]

tempid? is:

(defn ^boolean tempid? [x]
  (instance? TempId x))

I suspect the ? has nothing to do with it and it's the ^boolean type hint that's the issue.

Comment by Alex Miller [ 13/Jan/16 8:27 AM ]

This is a bug in Om in using a type hint in non-conditional code that is cljs-specific. David Nolen is fixing in Om.

Comment by Alex Miller [ 13/Jan/16 8:27 AM ]

https://github.com/omcljs/om/commit/cc39f37561455a54153aaef7e5ca36782839aa38

Comment by Geraldo Lopes de Souza [ 13/Jan/16 4:38 PM ]

Alex,

I saw the other issue referencing the ^boolean but I didn't relate with this.

Thank you,

Geraldo Lopes de Souza





[CLJ-1878] destructuring doesn't work on a sorted set Created: 08/Jan/16  Updated: 08/Jan/16  Resolved: 08/Jan/16

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

Type: Defect Priority: Major
Reporter: Anton Gladyshevskiy Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: collections


 Description   

Destructuring doesn't work on a sorted set:

(let [[head :as demoset] (sorted-set 1 2 3)] head)

UnsupportedOperationException nth not supported on this type: PersistentTreeSet clojure.lang.RT.nthFrom (RT.java:933)



 Comments   
Comment by Alex Miller [ 08/Jan/16 4:40 PM ]

Sequential destructuring works on sequential collections - sets (even sorted sets) are unordered (not sequential).

You can make this work with something like (which defines a sequential collection from the set):

(let [[head :as demoset] (seq (sorted-set 1 2 3))] head)
Comment by Anton Gladyshevskiy [ 08/Jan/16 4:57 PM ]

Thank you. It's a pity, because in this case 'demoset' becomes a sequence, not a sorted set.





[CLJ-1877] operation on the sorted-set fails under certain conditions Created: 08/Jan/16  Updated: 08/Jan/16  Resolved: 08/Jan/16

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

Type: Defect Priority: Major
Reporter: Anton Gladyshevskiy Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: collections
Environment:

Windows 8.1



 Description   

Function generates a sequence of prime numbers. Uses a priority queue implemented as a sorted set of vectors [priority val].

On the iteration (x = 10) fails with message:

(defn sieve [[x & t] pq]
  (lazy-seq
    (if (or (empty? pq) (< x (ffirst pq)))
      (cons x (sieve t (conj pq [(* x x) (next (iterate (partial + x) (* x x)))])))
      (sieve t
        (loop [pq pq] (let [[key val :as head] (first pq)]
          (if (= x key) (recur (conj (disj pq head) [(first val) (next val)])) pq) ))))))

(def primes (sieve (iterate inc 2) (sorted-set)))

(take 4 primes)
;; => (2 3 5 7)

(take 5 primes)
ClassCastException clojure.lang.Iterate cannot be cast to java.lang.Comparable  clojure.lang.Util.compare (Util.java:153)

When x = 10 it goes to the (loop ...) section and fails while trying to recur.



 Comments   
Comment by Alex Miller [ 08/Jan/16 4:45 PM ]

Sets are unordered (not sequential) and cannot be used with sequential destructuring. You can wrap (seq ) around the conj in the recur if you wish to make it sequential.

Comment by Anton Gladyshevskiy [ 08/Jan/16 4:55 PM ]

Destructuring is not a subject of this issue. The problem is that the code that have been successfully evaluated several times before fails on the subsequent iteration. I.e. when x is 4, 6, 8, 9 it works fine, but when x = 10 it fails.

Comment by Alex Miller [ 08/Jan/16 8:18 PM ]

Oh, yeah! I totally misread this one after reading the last one. The issue here is that you're trying to put something into a sorted set, but the type of the item (here the sequence produced by `iterate`) is not comparable to the existing items in the set (vectors). Vectors are comparable, but not general sequences.

A simpler example of the same issue is: `(conj (sorted-set) [1] '(2))`

There is a ticket for making lists comparable (CLJ-1467), but I'm not sure it would be a good idea to generically extend this to all sequential data types.





[CLJ-1869] EdnReader allows keywords starting with number Created: 18/Dec/15  Updated: 18/Dec/15  Resolved: 18/Dec/15

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

Type: Defect Priority: Minor
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

CLJ-1252 pointed out this problem with the LispReader, but the patch there was rejected because it broke too many things. The same problem exists with the EdnReader, but I would guess that patching it would not cause as much breakage. I suggest applying the CLJ-1252 patch to EdnReader.



 Comments   
Comment by Greg Chapman [ 18/Dec/15 2:35 PM ]

I apologize: for some reason I didn't notice that CLJ-1252 patched EdnReader as well as LispReader. So maybe changing EdnReader did cause breakage. If so, please ignore this report.

Comment by Alex Miller [ 18/Dec/15 2:53 PM ]

At this point, I believe we are likely to continue allowing keywords that start with a number. There is another ticket to sync up the spec with code (and actually it would be nice to fix the regex so it worked intentionally rather than accidentally).





[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-1855] Add a boolean? function Created: 24/Nov/15  Updated: 24/Nov/15  Resolved: 24/Nov/15

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

Type: Enhancement Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

It could be handy to have a boolean? function in core to match the checks for most other primitive Clojure types. Is this something that a patch would be considered for



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

Yes, although there is a bigger ticket (CLJ-1298) with suggestions for a number of type-related predicates. I would prefer to pass that list by Rich and have him yes/no things on it first prior to getting many individual patches.

boolean? is mentioned in the comments, but feel free to add it to the description to make that more prominent.

I'm going to dupe this to that one.

Comment by Daniel Compton [ 24/Nov/15 3:54 PM ]

Sorry about that. I searched for boolean? in JIRA and it didn't match any tickets so I thought this was a new request.





[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-1850] doseq expansion causes boxed math warning. Created: 13/Nov/15  Updated: 13/Nov/15  Resolved: 13/Nov/15

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

Type: Defect Priority: Trivial
Reporter: Michael Nygard Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: math


 Description   

When boxed math warnings are enabled, doseq causes a warning.



 Comments   
Comment by Alex Miller [ 13/Nov/15 2:27 PM ]

Example?

Comment by Michael Nygard [ 13/Nov/15 2:58 PM ]

Working on isolating a minimal example.

Comment by Michael Nygard [ 13/Nov/15 4:46 PM ]

With this source:

src/doseq_warning/core.clj
(ns doseq-warning.core
  (:require [clojure.core.async :as async]))

(set! *unchecked-math* :warn-on-boxed)

(defn example
  []
  (async/go-loop [actives []]
    (let [vch (async/alts! actives)]
      (doseq [c (second vch)]
        (async/close! c)))))

Using the following project.clj:

project.clj
(defproject doseq-warning "0.1.0-SNAPSHOT"
  :dependencies [[org.clojure/clojure "1.8.0-beta1"]
                 [org.clojure/core.async "0.1.346.0-17112a-alpha"]]
  :global-vars {*unchecked-math* :warn-on-boxed})

Then executing (load-file "src/doseq_warning/core.clj") at a REPL results in:

Boxed math warning, /Users/mtnygard/tmp/doseq-warning/src/doseq_warning/core.clj:8:3 - call: public static boolean clojure.lang.Numbers.lt(java.lang.Object,java.lang.Object). 
Boxed math warning, /Users/mtnygard/tmp/doseq-warning/src/doseq_warning/core.clj:8:3 - call: public static java.lang.Number clojure.lang.Numbers.unchecked_inc(java.lang.Object). 
#'doseq-warning.core/example
Comment by Alex Miller [ 13/Nov/15 4:49 PM ]

I think it's probably unlikely that this is an error with the boxed math warnings and more likely an artifact of using an older core.async with older tools.analyzer in the go block transformation.

Not reproducible with latest core.async, so closing.





[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-1847] clojure.walk/walk returns a PersistentVector when the input is of type IMapEntry Created: 13/Nov/15  Updated: 15/Nov/15  Resolved: 15/Nov/15

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

Type: Defect Priority: Minor
Reporter: Yehonathan Sharvit Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: walk

Attachments: Text File fix_walk.patch    
Patch: Code

 Description   

`walk` is supposed to build up a data structure of the same type as its input.
With `clojure.lang.IMapEntry`, it doesn't work as expected.
The output is of type: `clojure.lang.PersisentVector`

For instance,
(class (walk/walk identity identity (first {:a 1})) ); clojure.lang.PersisentVector



 Comments   
Comment by Alex Miller [ 15/Nov/15 12:54 PM ]

Thanks for filing this. I spent some time looking at it and I don't think there is anything that needs to be done at this time.

Re your description, "`walk` is supposed to build up a data structure of the same type as its input." Regarding IMapEntry.... prior to 1.8, IMapEntry concrete types were descendants of AMapEntry, in particular the most common concrete type was MapEntry, which were used in PersistentHashMap.

In 1.8, PHM now returns PersistentVectors, which now implement IMapEntry. So, walk now takes a map entry (which is a PV) and returns a PV, which is an IMapEntry. So I believe the contract is still satisfied.

The new map-entry? predicate can be used to catch only 2-element PVs (not other counts) as map entries, however it is important to still consider whether you will incorrectly snare 2 element vectors that aren't in a map in this particular case. The current clojure.walk code however will have the same effective result in either case so the clojure.walk code does not need to change (and in fact non-PV entries still exist in sorted maps).





[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-1842] Use code generation to support more than 4 primitive arguments in function calls Created: 02/Nov/15  Updated: 02/Nov/15  Resolved: 02/Nov/15

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

Type: Enhancement Priority: Minor
Reporter: Richard Davies Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: compiler
Environment:

All



 Description   

The current restriction that "fns taking primitives support only 4 or fewer args" is apparently to prevent an explosion of possible interfaces https://groups.google.com/forum/#!topic/clojure/MI7iakMqEXo . Could the same code generation approach to "Unrolled small vectors" (http://dev.clojure.org/jira/browse/CLJ-1517) be used to increase the supported arities of primitive functions in IFn.java? I have bumped into this restriction a few times when trying to tune my code for high performance.

Each additional arity would require (1 + arg-arity)^3 interfaces to be generated. I understand that it is possible to embed primitives within deftypes instead of passing more parameters. However, the main use case for type-hinting to generate primitive interfaces is when an increase in performance is required so the deftype work-around is not optimal. Likewise, it is possible to drop out to Java to implement the primitive functions but this complicates the development cycle/tools/etc.



 Comments   
Comment by Alex Miller [ 02/Nov/15 7:53 PM ]

The issue is not generating the interfaces (the existing interfaces were themselves generated), but whether the result is manageable in terms of code size etc. There are other ways to approach it though and we may do something in the future. Declining the ticket as we would not work it off of here.





[CLJ-1835] Fix minor typos in documentation Created: 28/Oct/15  Updated: 28/Oct/15  Resolved: 28/Oct/15

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

Type: Enhancement Priority: Trivial
Reporter: Artur Cygan Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: documentation
Environment:

Not relevant


Attachments: Text File 0001-PATCH-Fix-typos-iff-if-in-docstrings-and-comment.patch    

 Description   

iff -> if



 Comments   
Comment by Nicola Mometto [ 28/Oct/15 6:07 AM ]

Those are not typos, iff means "if and only if"

Comment by Artur Cygan [ 28/Oct/15 6:11 AM ]

Ah okay, didn't know that.





[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-1833] pretty-print fix needs type hint Created: 26/Oct/15  Updated: 26/Oct/15  Resolved: 26/Oct/15

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

Type: Enhancement Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: pprint

Attachments: Text File CLJ-1833-type-hint-in-pretty-writer.patch    

 Description   

A recent fix to the pretty-print code is missing a type hint. Other recent fixes turned on reflection warnings so now you get this warning when building Clojure:

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



 Comments   
Comment by Steve Miner [ 26/Oct/15 9:46 AM ]

The original fix was CLJ-1390. It was missing a type hint. (My fault.)

Comment by Steve Miner [ 26/Oct/15 9:47 AM ]

added type hint

Comment by Alex Miller [ 26/Oct/15 9:47 AM ]

Dupe of CLJ-1827





[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-1830] Test equality ignore decimal Created: 18/Oct/15  Updated: 19/Oct/15  Resolved: 18/Oct/15

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

Type: Enhancement Priority: Minor
Reporter: Nick DeCoursin Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   
user> (= {:a 1.00} {:a 1.0})
true
user> (= {:a 1} {:a 1.0})
false

This ^ is expected because (not (= 1 1.0)), so instead == is used to compare number equivalence: (== 1 1.0). But, == fails on collections:

user> (== {:a 1} {:a 1.0})
ClassCastException clojure.lang.PersistentArrayMap cannot be cast to java.lang.Number  clojure.lang.Numbers.equiv (Numbers.java:208)

In summary, there's not way to make this assertion (= {:a 1} {:a 1.0})



 Comments   
Comment by Alex Miller [ 18/Oct/15 4:50 PM ]

This would require a significant number of changes across the collection hierarchy to define a new additional kind of equivalence. I do not expect that we will add this functionality. If we did embark on this, it would be done through a design effort, not through a ticket like this. Thanks for the suggestion.

Comment by Andy Fingerhut [ 18/Oct/15 5:13 PM ]

Nick: Clojure core members make the final calls on things like this, but I am pretty sure that (= 1 1.0) is false by design. The inability to use == to compare anything other than numbers is also by design.

If you want a function, which for sake of example I will call "deep==" here, that uses == on numbers inside of collections, or collections nested inside of other collections, etc., I don't think it would be difficult to write one, as long as the values you wanted to compare using == were the values in the maps only, and not the keys.

If you wanted a function where (deep== {1 :a} {1.0 :a}) returned true, you would have to do something other than the normal key lookup functions built into Clojure, because they use clojure.core/hash to put items into 'hash buckets'. (clojure.core/hash x) and (clojure.core/hash (* 1.0 x)) are in general different from each other, again I believe by design.

Comment by Nick DeCoursin [ 19/Oct/15 1:34 AM ]

Thank you for looking at this, for your input, and the details.

I think a design change might be worth it. This would probably need to happen at 2.0 or something. But this is pretty fundamental that can serious hidden bugs.

Anyways, I don't mean to start a debate, but it's something that I think deserves some consideration. Thank you.





[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-1828] Remove trailing whitespace in clojure.test Created: 13/Oct/15  Updated: 13/Oct/15  Resolved: 13/Oct/15

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

Type: Enhancement Priority: Trivial
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: Text File clojure-test-whitespace.patch    

 Description   

Removes trailing whitespace from clojure.test.



 Comments   
Comment by Daniel Compton [ 13/Oct/15 9:00 PM ]

Declined by Alex on Slack.





[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-1824] Splicing macros Created: 12/Oct/15  Updated: 14/Oct/15  Resolved: 14/Oct/15

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

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


 Description   

In many cases, it would be convenient for a macro to "splice" its return value into the resulting form.

Example use cases:

  • `comment` can return no forms, so that it can be inserted in places where nil would be disruptive
  • a macro can return two forms to create a condition in a `cond` block
  • a macro can create multiple forms to support a variable-arity function (e.g. passing a set of keyword arguments)
  • a macro can create one or more complete `binding`, `let` or `loop` bindings

Proposed syntax and usage:

(defmacro-splicing multi-test [v values exp]
  (mapcat 
    (fn [value] `[(= ~v ~value) ~exp]`
    values))

(let [v (compute-some-result)]
  (cond 
    (multi-test v [1 3 5 7 9] "Odd digit")
    (multi-test v [0 2 4 6 8] "Even digit")
    :else "Not a digit"))


 Comments   
Comment by Ghadi Shayban [ 13/Oct/15 7:08 PM ]

These sorts of things need a design discussion prior to a JIRA ticket.

Comment by Alex Miller [ 14/Oct/15 8:00 AM ]

I'm declining this as a ticket as it does really need design evaluation in some way before it gets to this point (either clojure-dev mailing list or a design wiki page or something).

I'm not passing any judgement on the idea, which is potentially interesting.





[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-1819] Add removev function Created: 28/Sep/15  Updated: 28/Sep/15  Resolved: 28/Sep/15

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

Type: Enhancement Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

We already have mapv and filterv from http://dev.clojure.org/jira/browse/CLJ-847. What is Core's opinion on adding removev to complement filterv?



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

I do not expect that the set of vector fns will expand. Use transducers instead:

(into [] (remove odd?) [1 2 3 4 5])




[CLJ-1816] Allow AssertionError messages for function :pre and :post conditions to be specified. Created: 23/Sep/15  Updated: 28/Sep/15  Resolved: 28/Sep/15

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

Type: Enhancement Priority: Trivial
Reporter: Tristan Strange Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None
Environment:

All Clojure platforms



 Description   

A failing in a predicate in a list of :pre or :post conditions currently causes messages similar to one below to be displayed:
{{(defn must-be-a-map [m] {:pre [(map? m)]} m)
(must-be-a-map [])

AssertionError Assert failed: (map? m) user/must-be-a-map (form-init.....clj:1)}}

These exception messages could be made significantly more descriptive by allowing specific messages strings to be associated with each predicate in :pre and :post conditions.

Predicate functions and there associated messages strings could be specified as a pair of values in a map:
{{
(defn must-be-a-map
[m]
{:pre [{(map? m) "m must be a map due to some domain specific reason."}]}
m)}}

The following would then produce an error message as follows:
{{(must-be-a-map 10)
AssertionError Assert failed: m must be a map due to some domain specific reason.
(map? m) user/must-be-a-map (form-init.....clj:1)}}

This would allow predicates without messages to specified alongside pairs of associated predicate message pairs as follows:
{{(defn n-and-m [n m] {:pre [(number? n) {(map? m) "You must provide a map!"}]})}}

This change would not break any existing functionality and still allow for predicates to be predefined elsewhere in code.

As a result pre and post conditions could provide a natural means of further documenting the ins and outs of a function, simplify the process of providing meaningful output when developing libraries and perhaps make the language better suited to teaching environments[1]

[1] http://wiki.science.ru.nl/tfpie/images/2/22/TFPIE2013_Steps_Towards_Teaching_Clojure.pdf



 Comments   
Comment by Tristan Strange [ 23/Sep/15 6:55 PM ]

Many apologies this is a duplicate of 1817! Confusion over create and preview buttons was the problem! Many apologies.

Comment by Alex Miller [ 28/Sep/15 9:53 AM ]

CLJ-1817





[CLJ-1815] select-keys should throw when not called with a map Created: 21/Sep/15  Updated: 21/Sep/15  Resolved: 21/Sep/15

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

Type: Enhancement Priority: Trivial
Reporter: Michiel Borkent Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: maps


 Description   

Currently select-keys accepts associative data structures that are not maps:

(select-keys [:a :b] [:a]) ;;=> {}
(select-keys [:a :b] [0]) ;;=> {0 :a}

I think select-keys should just throw when not called with a map.



 Comments   
Comment by Alex Miller [ 21/Sep/15 9:15 AM ]

vectors are associative and select-keys works with any map type so this is expected behavior.





[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-1802] StackTraceElement's FileName null Created: 21/Aug/15  Updated: 21/Aug/15  Resolved: 21/Aug/15

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

Type: Defect Priority: Critical
Reporter: Amir Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: None
Environment:

linux



 Description   

I tried DBAppender from logback but after logging 16-20 events no event was being logged in Database, when i debuged the app i came to know that FileName from StackTraceElement is null which is raising the following exception

java.sql.SQLIntegrityConstraintViolationException: ORA-01400: cannot insert NULL into ("XYZ"."LOGGING_EVENT"."CALLER_FILENAME")

I made a simple application in java and issue was not present there.

The DBAppender of lobback fills information about log event in method bindCallerDataWithPreparedStatement.

i suspect clojure is not passing the information completely sometimes. in this case when i call the following

(dotimes[x 100]
(logr/infor "abcd" x))

please keep in mind it happens when using a marker

<appender name="REPORTING-DB" class="ch.qos.logback.classic.db.DBAppender">
<filter class="ch.qos.logback.core.filter.EvaluatorFilter">
<evaluator class="ch.qos.logback.classic.boolex.OnMarkerEvaluator">
<marker>DB_REPORT</marker>
</evaluator>
<onMismatch>DENY</onMismatch>
<onMatch>ACCEPT</onMatch>
</filter>

<connectionSource class="ch.qos.logback.core.db.DataSourceConnectionSource">
<dataSource class="com.zaxxer.hikari.HikariDataSource">
<!-- <dataSource class="com.mchange.v2.c3p0.ComboPooledDataSource"> -->
<driverClassName>oracle.jdbc.driver.OracleDriver</driverClassName>
<jdbcUrl>jdbc:oracle:thin:xxxx/xxxx@xdomain:1521:EDB
</jdbcUrl>
<user>xxxx</user>
<password>xxxx</password>
</dataSource>
</connectionSource>
</appender>

<appender name="FILE" class="ch.qos.logback.core.FileAppender">
<file>test.log</file>
<append>true</append>
<encoder>
<pattern>%-4relative [%thread] %-5level %logger{35} - %msg%n
</pattern>
</encoder>
</appender>

<root level="INFO">
<appender-ref ref="REPORTING-DB" />
<appender-ref ref="FILE" />
</root>



 Comments   
Comment by Amir [ 21/Aug/15 5:27 AM ]

(defmacro infor [ & args ]
`(let logger# (impl/get-logger logging/*logger-factory* ~*ns*)
(println (class logger#))
(when (impl/enabled? logger# :info)
(.info logger# DB_REPORT (print-str ~@args)))))

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

First, there is not enough info here to reproduce the problem apart from the logging framework and oracle db.

Second, StackTraceElement's getFileName() is allowed to return null, so this seems like a faulty assumption in the database.

If there is a specific example (with code to reproduce) where a stack trace is missing a file name, but one could have been provided, then this could be reopened.





[CLJ-1801] Boxed Boolean values break `if` special form Created: 13/Aug/15  Updated: 13/Aug/15  Resolved: 13/Aug/15

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

Type: Defect Priority: Critical
Reporter: Adrian Medina Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: None


 Description   

We came across this in production code where at some point a value in a map (e.g., {:some-key false}) was being used as the test form of a conditional statement which was evaluating counter-intuitively. After much bewilderment, we figured out that it was because somewhere along the line the literal false value was being boxed. You can reproduce this by evaluating the following form in a REPL:

(if (Boolean. false)
true
false)

=> true



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

Not a bug in Clojure. See [1] Make sure the library you are using does the proper canonicalization of boolean [2].

[1] http://clojure.org/special_forms#toc2

[2] https://github.com/ghadishayban/squee/blob/master/src/squee/impl/resultset.clj#L66-L69

Comment by Alex Miller [ 13/Aug/15 3:08 PM ]

Long ago, it was decided that only the canonical Boolean/FALSE value would be considered false in Clojure and there are no plans to change this.

Comment by Adrian Medina [ 13/Aug/15 3:15 PM ]

This is really not considered a bug? I didn't mean to imply we were using boxed booleans purposefully - in fact we weren't (the map in question get assoc'd a literal false value), and I had no idea the boxing was occurring until I dug deeper into the bug. The printed representation of a boxed boolean false value is false making debugging this issue very difficult. Perhaps the printed representation of a boxed boolean value should be changed?

Comment by Nicola Mometto [ 13/Aug/15 5:42 PM ]

Boolean/FALSE and Boolean/TRUE are already boxed booleans so your code must be constructing a new boxed boolean and using that.





[CLJ-1795] Protocol functions don't work properly when metadata is added to them Created: 08/Aug/15  Updated: 10/Aug/15  Resolved: 10/Aug/15

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

Type: Defect Priority: Major
Reporter: Nathan Marz Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: metadata, protocols
Environment:

Clojure 1.7.0



 Description   

When you add metadata to a protocol function, the version with metadata will not work for any extensions added afterwards.

(defprotocol TestProtocol
  (tester [o]))

(def tester-with-meta (with-meta tester {:a 1}))

(extend-protocol TestProtocol
  String
  (tester [o] (println "Strings work!")))

(tester-with-meta "A") ;; Error
(tester "A") ;; Works fine

(def tester-with-meta (with-meta tester {:a 1}))

(extend-protocol TestProtocol
  Long
  (tester [o] (println "Longs work!")))

(tester-with-meta "A") ;; Works fine
(tester-with-meta 3) ;; Error


 Comments   
Comment by Alex Miller [ 08/Aug/15 9:16 AM ]

Can you specify version you're testing with too...

Comment by Nathan Marz [ 08/Aug/15 9:21 AM ]

Clojure 1.7.0

Comment by Nathan Marz [ 08/Aug/15 10:53 AM ]

This is subsumed by http://dev.clojure.org/jira/browse/CLJ-1796 which seems to be closer to the root cause

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

Subsumed by CLJ-1796





[CLJ-1788] Integrate lein with the clojure distribution. Created: 27/Jul/15  Updated: 27/Jul/15  Resolved: 27/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Morten Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: enhancement, newbie
Environment:

All



 Description   

As a new clojure user I think clojure would be more approachable by beginners and less confusing to get started with if "lein" could get included in the standard clojure distribtion.

It seems almost all tools, books and clojure users use "lein" anyway so why make it a seperate download? It may seem as a trivial issue for existing power clojure developers but offering a integrated clojure distribution with a official package manager etc. would make it easier for new users to get started. After all clojure is not the simplest thing to get started with and "low hanging fruits" like this could help a lot. For tool vendors there would also be a benefit if "lein" was included.



 Comments   
Comment by Alex Miller [ 27/Jul/15 1:05 PM ]

This is not something we're going to do.

Comment by Morten [ 27/Jul/15 1:23 PM ]

I am new so I don't know how things are decided here but can I politely ask why? As a new user I can firmly say it would have helped me (who did not know about lein when I first got started) and is helping get new users easily started not a worthwhile goal (especially if it is easily done) ?

Comment by Andy Fingerhut [ 27/Jul/15 1:38 PM ]

Morten, I can't comment officially on what changes will or will not be made to Clojure.

I did want to point out that if you get Leiningen, you don't need to do a separate manual download of Clojure in addition to that. Running Leiningen the first time auto-downloads it and a few other things for you.

Leiningen is not the only way to run Clojure, so the Clojure getting started page here: http://clojure.org/getting_started mentions Leiningen as one possible approach, but it does not happen to mention that if you choose that approach, downloading the JAR as described at the top of that page is unnecessary. Might be nice if that page mentioned that fact about Leiningen.

Comment by Morten [ 27/Jul/15 1:55 PM ]

I downloaded clojure first and only when I found out that my IDE insisted on a project file that belongs to "leiningen" did I find and install lein.

As for leining just being one approach I did read on InfoQ that according to a recent survey "Leiningen, at a whopping 98% adoption rate," (among Clojure users I assume).

So the 98% users seems to make lein a defacto standard and the fact that the IDE's I have tried require leining to work with clojure make another compelling argument.

But anyway - it is up to the clojure dev community to decide. I am just a new users and just wondered at the very quick and prompt No without any readon why?

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

Leiningen is an external tool built with a different contribution model and set of goals than Clojure itself. It's also not the only build tool in use in the Clojure community. As such, there are no plans to bundle Leiningen into Clojure.





[CLJ-1786] Unused defaults are evaluted in `:or` destructoring Created: 26/Jul/15  Updated: 26/Jul/15  Resolved: 26/Jul/15

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

Type: Defect Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: compiler


 Description   
(defn example [{:keys [foo] :or {foo (println "Evaluated!")}}])

(example {:foo 42})
;; prints "Evaluated!"


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

Dupe of CLJ-1676





[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-1783] Remove unnecessary call to seq() in clojure.lang.Cons.next() Created: 21/Jul/15  Updated: 22/Jul/15  Resolved: 21/Jul/15

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

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

Attachments: Text File clj-1783.patch    

 Description   

Currently clojure.lang.Cons has the following implementation for next():

public ISeq next(){
	return more().seq();
}

This appears to be unnecessary and could be replaced with the following, since _more is already an ISeq

public ISeq next(){
	return _more;
}

There are minor performance gains from this, because:
a) It avoids an unnecessary null check
b) In the null case it avoids an unnecessary call to PersistentList.EMPTY.seq() (which is guaranteed to return null, although the JVM probably optimises this away)
c) In the non-null case it avoids an unnecessary interface dispatch call to ISeq.seq()

It is possible that this change affects laziness, since the value contained in _more could theoretically perform some processing opon the invocation of `seq`. However, any such change should be an improvement since it increases rather than reduces laziness.



 Comments   
Comment by Jozef Wagner [ 21/Jul/15 5:46 AM ]

Have all tests passed? IMO your patch would break (assert (nil? (next (cons 1 (lazy-seq nil))))).

next must call seq(), because it promises to return nil for empty Seqs.

Comment by Mike Anderson [ 21/Jul/15 6:00 AM ]

Hmmmm that is an interesting point Jozef. I hadn't realised that there are valid cases where ISeq instances are allowed to be both non-nil and empty.

Will have to give more thought to see if these cases can be handled.

Comment by Mike Anderson [ 21/Jul/15 10:21 PM ]

Closing because I don't think this can be resolved without more invasive changes.

For future reference, it seems a bit unfortunate that non-null ISeq instances are allowed to be empty. Allowing for this seems to create a requirement for a lot of unnecessary seq() calls throughout the Clojure code base.

It seems like it might be better if:
a) ISeq instances were constrained to be non-empty or nil
b) Objects representing potentially empty sequences (LazySeq etc.) are simple Sequable / Sequentional, not ISeq instances

I don't think this is easily fixable at this point however.

Comment by Mike Anderson [ 21/Jul/15 10:22 PM ]

Probably not fixable without invasive changes across the code base

Comment by Kevin Downey [ 22/Jul/15 12:24 PM ]

historical note:

at one point clojure did not have empty seqs, it had seqs with stuff in them and nil. in order to achieve this the laziness was of a slightly different character, seqs effectively called `seq` on themselves when created so either at least the first element was realized, or you got nil. a blog post or two came out discussing the nature of the laziness of seqs, and I think one even turned its nose up at any kind of laziness that wasn't completely lazy, and some time after that Rich changed the laziness of lazy seqs. If you look at #clojure's irc log around this time you'll see lots of discussion about nil-punning being broken, and a brief window when `if` was a macro that expanded in to `if*` and the `if` macro checked for broken nil punning. I don't recall exactly but I think that was sometime in 2009.

this also is where `next` came from. clojure had `first` and `rest`, but now `rest` could return an empty seq, so if you wanted to continue nil punning and not worry about empty seqs, `next` does the same thing as `rest`, but it never returns an empty seq, so its result can be safely used in an `if`





[CLJ-1781] Tuples don't extend IKVReduce Created: 19/Jul/15  Updated: 30/Jul/15  Resolved: 30/Jul/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: Declined Votes: 0
Labels: regression
Environment:

1.8.0-alpha1 or 1.8.0-alpha2


Approval: Vetted

 Description   

This is a regression from the tuple stuff (both return nil in 1.7):

(reduce-kv (fn [acc idx in] acc) nil [1 2 3 4 5 6 7]) ; => nil
(reduce-kv (fn [acc idx in] acc) nil [1 2])
;; =>  No implementation of method: :kv-reduce of protocol:
;; #'clojure.core.protocols/IKVReduce found for class: clojure.lang.Tuple$T2


 Comments   
Comment by Michael Blume [ 19/Jul/15 11:47 AM ]

CLJ-1689 would sort this.

Comment by Alex Miller [ 30/Jul/15 1:14 PM ]

Since tuples were pulled out in 1.8.0-alpha3, declining.





[CLJ-1780] Test OOME from locals clearing change Created: 17/Jul/15  Updated: 21/Aug/15  Resolved: 21/Aug/15

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

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: regression
Environment:

1.8.0-alpha2


Attachments: Text File strengthen-clearing-test.patch    
Approval: Triaged

 Description   

IBM JDK 1.6 in test matrix is throwing errors from the new test for CLJ-1250 in 1.8.0-alpha2.

[java] ERROR in (test-closed-over-clearing) (Range.java:133)
     [java] expected: (number? (reduce + 0 (r/map identity (range 1.0E8))))
     [java]   actual: java.lang.OutOfMemoryError: null
     [java]  at clojure.lang.Range.forceChunk (Range.java:133)
     [java]     clojure.lang.Range.chunkedFirst (Range.java:150)
     [java]     clojure.core$chunk_first.invoke (core.clj:667)
     [java]     clojure.core.protocols/fn (protocols.clj:136)
     [java]     clojure.core.protocols$fn__6478$G__6473__6487.invoke (protocols.clj:19)
     [java]     clojure.core.protocols$seq_reduce.invoke (protocols.clj:31)
     [java]     clojure.core.protocols/fn (protocols.clj:95)
     [java]     clojure.core.protocols$fn__6452$G__6447__6465.invoke (protocols.clj:13)
     [java]     clojure.core.reducers$folder$reify__21452.coll_reduce (reducers.clj:126)
     [java]     clojure.core$reduce.invoke (core.clj:6519)
     [java]     clojure.test_clojure.reducers/fn (reducers.clj:95)
     [java]     clojure.test$test_var$fn__7669.invoke (test.clj:703)
     [java]     clojure.test$test_var.invoke (test.clj:703)
     [java]     clojure.test$test_vars$fn__7691$fn__7696.invoke (test.clj:721)
     [java]     clojure.test$default_fixture.invoke (test.clj:673)
     [java]     clojure.test$test_vars$fn__7691.invoke (test.clj:721)
     [java]     clojure.test$default_fixture.invoke (test.clj:673)
     [java]     clojure.test$test_vars.invoke (test.clj:717)
     [java]     clojure.test$test_all_vars.invoke (test.clj:727)
     [java]     clojure.test$test_ns.invoke (test.clj:746)
     [java]     clojure.core$map$fn__4553.invoke (core.clj:2624)
     [java]     clojure.lang.LazySeq.sval (LazySeq.java:40)
     [java]     clojure.lang.LazySeq.seq (LazySeq.java:49)
     [java]     clojure.lang.Cons.next (Cons.java:39)
     [java]     clojure.lang.RT.next (RT.java:681)
     [java]     clojure.core/next (core.clj:64)
     [java]     clojure.core$reduce1.invoke (core.clj:909)
     [java]     clojure.core$reduce1.invoke (core.clj:900)
     [java]     clojure.core$merge_with.doInvoke (core.clj:2936)
     [java]     clojure.lang.RestFn.applyTo (RestFn.java:139)
     [java]     clojure.core$apply.invoke (core.clj:632)
     [java]     clojure.test$run_tests.doInvoke (test.clj:761)
     [java]     clojure.lang.RestFn.applyTo (RestFn.java:137)
     [java]     clojure.core$apply.invoke (core.clj:630)
     [java]     user$eval28810.invoke (run_test.clj:8)
     [java]     clojure.lang.Compiler.eval (Compiler.java:6850)


 Comments   
Comment by Nicola Mometto [ 18/Jul/15 12:02 AM ]

I don't understand how forcing a 32 element chunk could consume the memory. If locals clearing worked properly there should be no other part of that sequence in memory but I might be missing some detail.

Might there be something going on with the recent change in impl for Range?

Comment by Ghadi Shayban [ 18/Jul/15 12:19 AM ]

An interesting note is that (reduce + 0 (r/map identity (range 1e8))) is going through the seq path, despite reducers && reducible range. This is because coll-reduce doesn't prefer IReduceInit.

The CLJ-1250 test should be modified to intentionally hold the head of a seq in order to exercise the locals clearing. A good hypothesis from Alex is that GC is a bit slower on the archaic IBM JDK.

Comment by Ghadi Shayban [ 18/Jul/15 1:21 AM ]

I can't reproduce this on Linux x86-64 with IBM JDK and an artificially tiny heap.

[ghadi@amdhex ibm-java-x86_64-60]$ ./bin/java -Xmx128m -cp $HOME/jsr166y-1.7.0.jar:$HOME/clojure-1.8.0-master-SNAPSHOT.jar clojure.main
Clojure 1.8.0-master-SNAPSHOT
user=> (require '[clojure.core.reducers :as r])
nil
user=> (number? (reduce + 0 (r/map identity (range 1e8))))
true

Can you post details on the IBM JDK environment in CI? Need point release, heap size, kernel, distro, and jvm opts.

I've strengthened the CLJ-1250 test case by relying on neither reducer impl nor range impl, and I reverified that the bug is in fact present on <1.7 and gone on -master using Oracle JDK and 128m heap.

Comment by Alex Miller [ 18/Jul/15 8:52 AM ]

OS is CentOS 5.5 afaict

JDK details:

java version "1.6.0"
Java(TM) SE Runtime Environment (build pxa6460sr9fp2-20110625_01(SR9 FP2))
IBM J9 VM (build 2.4, JRE 1.6.0 IBM J9 2.4 Linux amd64-64 jvmxa6460sr9-20110624_85526 (JIT enabled, AOT enabled)
J9VM - 20110624_085526
JIT - r9_20101028_17488ifx17
GC - 20101027_AA)
JCL - 20110530_01

As far as I can tell, nothing is being done to alter the default heap size or other jvm opts during the build.

Comment by Ghadi Shayban [ 18/Jul/15 2:27 PM ]

The IBM JDK6 version I used was:

java version "1.6.0"
Java(TM) SE Runtime Environment (build pxa6460sr16fp7-20150708_01(SR16 FP7))
IBM J9 VM (build 2.4, JRE 1.6.0 IBM J9 2.4 Linux amd64-64 jvmxa6460sr16fp7-20150701_255724 (JIT enabled, AOT enabled)
J9VM - 20150701_255724
JIT - r9_20150630_95420
GC - GA24_Java6_SR16_20150701_1008_B255724)
JCL - 20150628_01

Seems like SR16 FP7 == 6.0.16.7, and the one on the CI build is SR9 FP2 == 6.0.9.2, a four or five year difference between point releases.

Comment by Ghadi Shayban [ 18/Jul/15 2:51 PM ]

OK I found a download for the (old) IBM JDK 6.0.9.2 and installed it on Linux x86-64. I cannot reproduce the bug.

Comment by Alex Miller [ 18/Jul/15 3:53 PM ]

I'd be happy to update the ibm jdk 1.6 version and n the build box too to see what happens.

Comment by Alex Miller [ 21/Aug/15 3:09 AM ]

Since the CLJ-1250 patch has been reverted, this is no longer failing in the build and I'm going to close it for now. Will reopen if necessary when CLJ-1793 (the updated version of CLJ-1250) goes in.





[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-1773] Support for REPL commands for tooling Created: 01/Jul/15  Updated: 09/Oct/15  Resolved: 11/Aug/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: Duplicate Votes: 0
Labels: repl


 Description   

Per http://dev.clojure.org/display/design/Socket+Server+REPL, want to enhance repl to support "commands" useful for nested repls or for parallel tooling repls communicating over sockets (CLJ-1671).

Commands are defined as keywords in the "repl" namespace. The REPL will trap these after reading but before evaluation. Currently this is a closed set, but perhaps it could be open - the server socket repl could then install new ones if desired when repl is invoked.

Commands:

  • :repl/quit - same as Ctrl-D but works in terminal environments where that's not feasible. Allows for backing out of a nested REPL.
  • :repl/push - push the current repl "state" (tbd what that is, but at least ns) to a stateful map in the runtime. Returns coordinates that can be used to retrieve it elsewhere
  • :repl/pull <coords> - retrieves the repl state defined by the coordinates

In the tooling scenario, it is expected that there are two repls - the client repl and the tooling repl. The tooling can send :repl/push over the client repl before startup and retrieve the coordinates (which don't change). Then the tooling repl can call :repl/pull at any time to retrieve the state of the client repl.



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

I originally was trying to split up the stuff in the socket repl ticket with this but so far it has been far easier to work on them in tandem, so I'm going to just dupe this into that one (CLJ-1671).

Comment by Alex Miller [ 09/Oct/15 11:25 AM ]

folded into other ticket





[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-1760] Add `partial` reader macro Created: 17/Jun/15  Updated: 22/Jan/16  Resolved: 22/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Mario T. Lanza Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: reader, tacit-programming


 Description   

One of the most common things one does in functional programming is partial application. Clojure doesn't curry its functions as Haskell does. Instead it offers `partial` and the function macro:

(def hundred-times (partial * 100))
(def hundred-times #(* 100 %))

While the function macro is both terse and flexible it doesn't offer the same feel that partial does when it comes to [tacit style](https://en.wikipedia.org/wiki/Tacit_programming). Using `partial` regularly, however, defeats the brevity one would otherwise enjoy in point-free style. Having a `partial` reader macro, while seemingly a small thing, would better lend itself to the tacit style.

(def hundred-times #%(* 100))

Because most functions list arguments from general to specific, I rarely need to use the function macro to place the optional argument in some position other than last – e.g. normal partial application.



 Comments   
Comment by Mario T. Lanza [ 27/Jun/15 2:08 PM ]

Just wanted to note that others had suggested the same idea albeit using another implementation.

http://stackoverflow.com/questions/18648301/concise-syntax-for-partial-in-clojure

Comment by Alex Miller [ 22/Jan/16 9:58 AM ]

I talked to Rich and he's not interested in this change, so declining.





[CLJ-1757] Inconsistent equals semantics for BigDecimal between = and .equals Created: 16/Jun/15  Updated: 22/Jun/15  Resolved: 22/Jun/15

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

Type: Defect Priority: Major
Reporter: Greg Mitchell Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: math
Environment:

RHEL5, VirtualBox



 Description   

Numbers.equiv for BigDecimal uses compareTo to test equality instead of equals. This means that = and .equals have different results when the scale of the two BigDecimals are different. For example:
=> (= 1.0M 1.00M)
true
=> (.equals 1.0M 1.00M)
false

I see that another JIRA (http://dev.clojure.org/jira/browse/CLJ-1118) changed this behavior and asserts it in unit tests, but this seems like very incorrect behavior.

The motivation for this issue is a unit test using clojure.data/diff to verify that a test message is the same as a message generated with my platform's code. Our downstream customers care about the scale of the decimal, so Clojure's = operator saying two decimals with a different scale are equal caused a difficult-to-detect bug.

For reference, the line causing the issue is here: https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Numbers.java#L964
And a test asserting this behavior is here: https://github.com/clojure/clojure/blob/master/test/clojure/test_clojure/numbers.clj#L75
Javadoc for BigDecimal: https://docs.oracle.com/javase/7/docs/api/java/math/BigDecimal.html#equals(java.lang.Object)



 Comments   
Comment by Greg Mitchell [ 22/Jun/15 12:42 PM ]

I haven't seen any action on this Jira. Can I provide anything else to facilitate this? I'd be happy to hear other peoples' opinions on if this is actually a bug. My team has tried to work around this issue in a couple of different ways, but re-implementing a diff which doesn't use = is a headache.

Comment by Andy Fingerhut [ 22/Jun/15 1:25 PM ]

Just to set your expectations, unless the Clojure core team considers a bug or enhancement critical, it is not unusual for a ticket to go for months with no changes or comments.

clojure.core/= and Java .equals are different for numeric arguments in many cases, by design. For example, clojure.core/= is true for numerically equal Byte, Short, Integer, Long, and BigInteger arguments, whereas Java .equals returns false if the types are different. I know this is not the issue you are raising in this ticket – it is just an example of one of many ways in which these things are different from each other.

Have you considered creating a modified version of clojure.data/diff that compares BigDecimal values in the way you prefer?

Comment by Alex Miller [ 22/Jun/15 1:29 PM ]

Greg, I haven't managed to get definitive feedback from Rich on this so I have not been able to update it either way. Certainly CLJ-1118 went through his review prior to being committed.

Comment by Greg Mitchell [ 22/Jun/15 2:42 PM ]

Andy - thanks, I didn't realize that. I haven't submitted a Jira to Clojure before. You have a good point, = does more type-coercion work for numerics. I believe that this is qualitatively different because the scale has important informational content in financial and scientific computing. == exists for cases where the type is less relevant, and the linked Jira seems like a reasonable solution for == specifically. As mentioned in my comment, we have tried a couple of ways to re-implement clojure.data/diff, but none of the easy or obvious solutions work. It relies quite deeply on the value-semantics for collections as implemented by = that short-circuits logic to handle BigDecimals specially.

Alex - Understood, thank you for the update.

Comment by Andy Fingerhut [ 22/Jun/15 3:23 PM ]

Greg, agreed that there are cases where clojure.core/= and clojure.core/== differ today, e.g. (== 1 1.0) is true but (= 1 1.0) is false.

If you are arguing that Clojure should change so that (= 1.0M 1.00M) is false, but (== 1.0M 1.00M) is still true, that seems reasonable, and perhaps CLJ-1118 went too far by making them not only == but also = (my bad, if so).

Comment by Alex Miller [ 22/Jun/15 4:19 PM ]

Rich confirmed that the current behavior is desired and we do not plan to make the suggested change.





[CLJ-1756] clojure.java.shell/sh fails with jvm 1.8 Created: 16/Jun/15  Updated: 16/Jun/15  Resolved: 16/Jun/15

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

Type: Defect Priority: Major
Reporter: john casu Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

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



 Description   

user=> (clojure.java.shell/sh "ls /sys/block")

IOException error=2, No such file or directory java.lang.UNIXProcess.forkAndExec (UNIXProcess.java:-2)



 Comments   
Comment by Alex Miller [ 16/Jun/15 4:20 PM ]

I think you want:

(clojure.java.shell/sh "ls" "/sys/block")
Comment by john casu [ 16/Jun/15 4:32 PM ]

I'm such a dumbass.
thanks.
-john

Comment by Alex Miller [ 16/Jun/15 4:37 PM ]

Naw, it's confusing (and inherited from Java).





[CLJ-1755] Calling nth on TransientVector with a default will not check if the transient has been made persistent Created: 16/Jun/15  Updated: 18/Jan/16  Resolved: 18/Jan/16

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

Type: Defect Priority: Major
Reporter: Patrick Gombert Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: transient

Attachments: Text File transient-vector-nth.patch    
Patch: Code
Approval: Triaged

 Description   

Invoking nth with arity two on a TransientVector will ensure that the transient is editable. However, invoking with arity three will return the supplied not-found value if the index is out of range.



 Comments   
Comment by Alex Miller [ 16/Jun/15 9:42 AM ]

Can you add an example to the description and a test to the patch?

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

An example of this would be something like:

(def t (transient ["a"]))
(persistent! t) ;; no longer editable
(nth t 0 :foo)  ;; IllegalAccessError Transient used after persistent! call
(nth t -1 :foo) ;; returns :foo (EXPECT: same IllegalAccessError)
(nth t 99 :foo) ;; IllegalAccessError as the inner count() call will throw it

Thus, the exposure here is solely on calling nth with a negative index on a no-longer transient collection, which returns the same answer as a persistent collection. This doesn't seem worth pursuing. Declining...





[CLJ-1753] VerifyError Expecting to find long on stack Created: 09/Jun/15  Updated: 09/Jun/15  Resolved: 09/Jun/15

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

Type: Defect Priority: Major
Reporter: Gerrit Jansen van Vuuren Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: bug


 Description   

The following functions cause a VerifyError when defining function m in the repl and via leiningen, it doesnt matter if AOT is enabled or not.
The cause is the function m's let statement (let [ts (select-ts msg)] ...)
If I take take out the select-ts's return type hint of ^long then everything works

(defn valid-ts? [^long ts] )
(defn ^long select-ts [msg] )

(defn m [msg] (let [ts (select-ts msg)] (when (valid-ts? ts) "hi")))

The full exception is:

java.lang.IllegalArgumentException: Unable to resolve classname: clojure.core$long@6df773ef
at clojure.lang.Compiler$HostExpr.tagToClass(Compiler.java:1069)
at clojure.lang.Compiler$InvokeExpr.getJavaClass(Compiler.java:3659)
at clojure.lang.Compiler$LocalBinding.hasJavaClass(Compiler.java:5657)
at clojure.lang.Compiler$LocalBindingExpr.hasJavaClass(Compiler.java:5751)
at clojure.lang.Compiler.maybePrimitiveType(Compiler.java:1283)
at clojure.lang.Compiler$MethodExpr.emitTypedArgs(Compiler.java:1336)
at clojure.lang.Compiler$InstanceMethodExpr.emit(Compiler.java:1523)
at clojure.lang.Compiler$IfExpr.doEmit(Compiler.java:2638)
at clojure.lang.Compiler$IfExpr.emit(Compiler.java:2613)
at clojure.lang.Compiler$BodyExpr.emit(Compiler.java:5826)
at clojure.lang.Compiler$LetExpr.doEmit(Compiler.java:6180)
at clojure.lang.Compiler$LetExpr.emit(Compiler.java:6133)
at clojure.lang.Compiler$BodyExpr.emit(Compiler.java:5826)
at clojure.lang.Compiler$FnMethod.doEmit(Compiler.java:5374)
at clojure.lang.Compiler$FnMethod.emit(Compiler.java:5232)
at clojure.lang.Compiler$FnExpr.emitMethods(Compiler.java:3771)
at clojure.lang.Compiler$ObjExpr.compile(Compiler.java:4410)
at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3904)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6642)
at clojure.lang.Compiler.analyze(Compiler.java:6445)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6632)
at clojure.lang.Compiler.analyze(Compiler.java:6445)
at clojure.lang.Compiler.access$100(Compiler.java:38)
at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:538)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6644)
at clojure.lang.Compiler.analyze(Compiler.java:6445)
at clojure.lang.Compiler.analyze(Compiler.java:6406)
at clojure.lang.Compiler.eval(Compiler.java:6707)
at clojure.lang.Compiler.eval(Compiler.java:6666)
at clojure.core$eval.invoke(core.clj:2927)
at clojure.main$repl$read_eval_print_6625$fn_6628.invoke(main.clj:239)
at clojure.main$repl$read_eval_print__6625.invoke(main.clj:239)
at clojure.main$repl$fn__6634.invoke(main.clj:257)
at clojure.main$repl.doInvoke(main.clj:257)
at clojure.lang.RestFn.invoke(RestFn.java:1096)
at clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__5879.invoke(interruptible_eval.clj:56)
at clojure.lang.AFn.applyToHelper(AFn.java:152)
at clojure.lang.AFn.applyTo(AFn.java:144)
at clojure.core$apply.invoke(core.clj:624)
at clojure.core$with_bindings_STAR_.doInvoke(core.clj:1862)
at clojure.lang.RestFn.invoke(RestFn.java:425)
at clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invoke(interruptible_eval.clj:41)
at clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn_5920$fn_5923.invoke(interruptible_eval.clj:171)
at clojure.core$comp$fn__4192.invoke(core.clj:2402)
at clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__5913.invoke(interruptible_eval.clj:138)
at clojure.lang.AFn.run(AFn.java:22)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
at java.lang.Thread.run(Thread.java:745)

VerifyError (class: user$m, method: invoke signature: (Ljava/lang/Object;)Ljava/lang/Object Expecting to find long on stack java.lang.Class.getDeclaredConstructors0 (Class.java:-2)



 Comments   
Comment by Alex Miller [ 09/Jun/15 4:21 PM ]

Maybe

(defn select-ts ^long [msg])

instead?

Comment by Alex Miller [ 09/Jun/15 4:23 PM ]

Where you have the ^long hint, it's resolved to the function clojure.core/long and the function is used as the typehint instead. We've got some other tickets to improve the feedback in this case.

Try:

(meta #'select-ts)

to see the meta better.

Comment by Gerrit Jansen van Vuuren [ 09/Jun/15 4:26 PM ]

that works.

what is the difference between (defn ^long select-ts [msg] ) and (defn select-ts ^long [msg]) ?

Comment by Alex Miller [ 09/Jun/15 4:28 PM ]

See CLJ-1674 for duplicate case w/boolean.

Comment by Gerrit Jansen van Vuuren [ 09/Jun/15 4:36 PM ]

thanks this explains it.





[CLJ-1751] realized? does not work on LongRange Created: 09/Jun/15  Updated: 09/Jun/15  Resolved: 09/Jun/15

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

Type: Defect Priority: Major
Reporter: Logan Linn Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

In pre-1.7 versions, all calls to range produced a LazySeq, but in 1.7.0-beta1 and up, if you call range with a end/start/step, you now get back a clojure.lang.LongRange which does not implement clojure.lang.IPending. Calling realized? on it throws exception.

Clojure 1.6.0
user=> (realized? (range 10))
false
Clojure 1.7.0-RC1

user=> (realized? (range 10))

ClassCastException clojure.lang.LongRange cannot be cast to clojure.lang.IPending  clojure.core/realized? (core.clj:7224)

clojure.lang.LongRange should implement clojure.lang.IPending



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

This is intentional and there have been a number of discussions and tickets about it already. I have chosen to interpret the meaning of whether a lazy seq is realized as whether the "first" value has been computed and can be returned without computation.

[Another possible interpretation is that a lazy seq is realized when there is a next. This is confusing because both the current value and the next node are forced at the same time as a result of most operations in LazySeq.]

For range, if a LongRange instance exists, its first value has been computed and is available. Thus it is not "pending" and always realized.

In general, sequences may be either IPending, or not. realized? only operates on IPending instances. Thus existing code that generically deals with sequences must have code like this (or it would be throwing ClassCastExceptions):

(defn seq-realized? [s]
  (if (instance? clojure.lang.IPending s)
    (realized? s)
    true))

Although this particular concrete range type has changed in terms of what it supports, generic code like that above will continue to work as before. cycle is the other case in 1.7 that falls into this case. Note that (range) is backed by iterate, which is IPending, so the answer there is actually different.

A separate, likely useful question is: should realized? return true for an instance that is not IPending? If so, then realized? could be used without the guard above. I don't believe that's been filed, but I'd support that change.

Also see: CLJ-1726

Comment by Logan Linn [ 09/Jun/15 1:57 PM ]

Thanks for the fast response, Alex. My apologies for not finding previous discussions prior to creating ticket.

Your points make sense and I realized (no pun intended) after I opened this that I shouldn't have specifically requested LongRange to implement IPending, but that we address behavior of realized? on non-infinite range. I'll file a separate ticket.





[CLJ-1749] evaluate quote with wrong number of arity will not throw any exception Created: 07/Jun/15  Updated: 08/Jun/15  Resolved: 07/Jun/15

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

Type: Defect Priority: Minor
Reporter: Di Xu Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: bug


 Description   

http://clojure.org/special_forms#Special Forms--(quote form)
doc says it should only accept one arity, and it also doesn't make sense if passes multiple arities, but in v1.6 and v1.7

user=> (quote 1 2 3)
1

I think it should throw exception, right?



 Comments   
Comment by Di Xu [ 07/Jun/15 9:39 PM ]

ps. can you guys fix hashtag in http://clojure.org/special_forms it's really hard to share

Comment by Alex Miller [ 07/Jun/15 9:40 PM ]

Dupe of CLJ-1282.

Comment by Alex Miller [ 08/Jun/15 1:10 AM ]

Re the link anchors on the special forms page, the table of contents at the top is actually generated from the headers, so I can't really change it unless I altered the headers more than I'd like. There are actually separate anchors embedded in the page like http://clojure.org/special_forms#quote which will work as well though.

Comment by Di Xu [ 08/Jun/15 1:18 AM ]

ok, thanks for that info. because I usually click link in the toc, and share that url, it's not that convenient.





[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-1742] Eduction doesn't implement IReduce Created: 01/Jun/15  Updated: 01/Jun/15  Resolved: 01/Jun/15

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

Type: Defect Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: Text File eduction.patch    
Patch: Code

 Description   

Eduction doesn't implement IReduce, hence

(reduce + (eduction [])) throws.



 Comments   
Comment by Ghadi Shayban [ 01/Jun/15 11:51 AM ]

I think this is by design. It has been brought up before that IReduce semantics aren't clear.

Comment by Alex Miller [ 01/Jun/15 1:05 PM ]

This is intentional. We are trying to lessen usage of reduce without init.





[CLJ-1740] Reader conditional #?@ only reads first form in some cases Created: 29/May/15  Updated: 29/May/15  Resolved: 29/May/15

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

Type: Defect Priority: Major
Reporter: Karsten Schmidt Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: bug, readerconditionals
Environment:

clojure1.7.0-beta3, java 1.8.0_25, osx 10.8.5



 Description   

The unsplicing reader conditional #?@ only reads the first form if it is read top-level:

(read-string {:read-cond :allow} "#?@(:clj [:a :b])")
;; :a

Whereas this produces the correct/expected result:

(read-string {:read-cond :allow} "(do #?@(:clj [:a :b]))")
;; (do :a :b)


 Comments   
Comment by Alex Miller [ 29/May/15 8:59 AM ]

This is a dupe of CLJ-1706 which was fixed in 1.7.0-RC1.

Comment by Karsten Schmidt [ 29/May/15 9:09 AM ]

Sorry, Alex! I did search for "reader conditional" but nothing relevant showed up... glad it's fixed! Thx!





[CLJ-1739] Broken set equality for sets of equal sets Created: 28/May/15  Updated: 28/May/15  Resolved: 28/May/15

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

Type: Defect Priority: Major
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: collections, interop


 Description   

With both clojure 1.6.0 and 1.7.0-RC1 I get the following inconsistent behavior.

Different kinds of sets are equal which is expected:

(= #{1 2 3} (flatland.ordered.set/ordered-set 1 2 3)) ;=> true
(= #{1 2 3} (java.util.HashSet. [1 2 3]))             ;=> true

However, sets containing equal sets are not equal:

(= #{#{1 2 3}} #{(flatland.ordered.set/ordered-set 1 2 3)}) ;=> false
(= #{#{1 2 3}} #{(java.util.HashSet. [1 2 3])})             ;=> false

This is similar to http://dev.clojure.org/jira/browse/CLJ-1649 and probably caused by http://dev.clojure.org/jira/browse/CLJ-1372.



 Comments   
Comment by Alex Miller [ 28/May/15 6:14 AM ]

This is a duplicate version of the problem in CLJ-1372. = will check that the persistent collection contains each element from the other collection, but because the hash codes are different for the Java version of the set, the element is not found and equality fails.

user=> (contains? #{#{1 2 3}} #{1 2 3})
true
user=> (contains? #{#{1 2 3}} (java.util.HashSet. [1 2 3]))
false
Comment by Andy Fingerhut [ 28/May/15 10:13 AM ]

There could be an issue where flatland.ordered.set/ordered-set is still using a pre-Clojure-1.6.0 hash function, and should be updated.

Comment by Tassilo Horn [ 28/May/15 2:26 PM ]

Andy, that's its hashCode implementation: https://github.com/flatland/ordered/blob/develop/src/flatland/ordered/set.clj#L52

Comment by Andy Fingerhut [ 28/May/15 2:40 PM ]

Yeah, that was correct with Clojure 1.5.1 and earlier. With Clojure 1.6.0, it should look more like what data.avl's was updated to around the time Clojure 1.6.0 was released, here: https://github.com/clojure/data.avl/blob/master/src/main/clojure/clojure/data/avl.clj#L53-L57

I will file an issue for ordered-set.

Comment by Andy Fingerhut [ 28/May/15 2:43 PM ]

https://github.com/amalloy/ordered/issues/16 was already filed recently. I added a comment with the same link to data.avl example there.

Comment by Andy Fingerhut [ 28/May/15 4:47 PM ]

I've created a PR for the issue with amalloy/ordered sets and maps: https://github.com/amalloy/ordered/pull/18





[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-1731] Transient maps can't be assoc!'d to contain more than 8 elements. Created: 17/May/15  Updated: 17/May/15  Resolved: 17/May/15

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

Type: Task Priority: Major
Reporter: Peter Herniman Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: assoc!, transient
Environment:

Linux, Fedora 20
Clojure 1.6.0
OpenJDK 64-Bit Server VM 1.7.0_79-mockbuild_2015_04_15_06_33-b00



 Description   

(let [x (transient {})] (dotimes [i 30] (assoc! x i 0)) (persistent! x))
Will result in

{0 0, 1 0, 2 0, 3 0, 4 0, 5 0, 6 0, 7 0}

instead of the expected 30 element map.

I'm not sure if this is fixed in the most recent version (development) but it doesn't work in 1.6.0.



 Comments   
Comment by Alex Miller [ 17/May/15 6:54 AM ]

This is not correct usage of transient maps. You must use the return value of assoc! for further updates, not bash the same instance. In other words, the update model is the same as with normal maps. There is an outstanding ticket to tweak the docstring slightly to make this clearer.

See a similar example with transient vectors at http://clojure.org/transients.

Comment by Peter Herniman [ 17/May/15 6:23 PM ]

Ah ok, looking at the example on clojure.org clears things up a lot.
I'm not too sure if the docstring does need updating, the tutorial on transients does state that you need to capture the return value explicitly.
Thanks!





[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-1718] (if test then else?) is inconsistent Created: 30/Apr/15  Updated: 05/May/15  Resolved: 30/Apr/15

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

Type: Defect Priority: Major
Reporter: Jigen Daisuke Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

MacOS X 10.10.2



 Description   

Issue CLJ-1640 was "fixed" by adding a comment to the documentation.
I think that's not enough, because CLJ-1640 actually points to a major inconsistency.
Just have a look at the following code:

;; I know, the definition of y is bad code, and I would
;; never write such rubish, BUT some people do (e.g. the
;; guys at MS, responsible for the sqljdbc4.jar JDBC
;; driver).
(def y (Boolean. false))

(if (= false y)
(println "that's ok"))

(if y
(println "that's inconsistent, because y is " y
" and we proved it with the above if statement"))



 Comments   
Comment by Alex Miller [ 30/Apr/15 3:25 PM ]

CLJ-1640 was closed by the submitter, no change was made for that.

The decision was made long ago by Clojure to canonicalize on Boolean/FALSE as the false value. We do not plan to make any changes in this regard.

In cases where you are dealing with Java libraries that construct new Boolean instances rather than use the canonical ones, you are responsible for normalizing these values. Please file issues with those libraries as appropriate.

Comment by Andy Fingerhut [ 30/Apr/15 3:41 PM ]

The longish example/article on ClojureDocs.org might contain additional useful info. Even Java recommends against ever using (Boolean. false): http://clojuredocs.org/clojure.core/if

Comment by Jigen Daisuke [ 30/Apr/15 5:08 PM ]

But, if Boolean/FALSE is the false value, shouldn't

(if (= (Boolean. false) false) true false)

better return false?

Comment by Andy Fingerhut [ 30/Apr/15 6:12 PM ]

These all return 2:

(if nil 1 2)
(if false 1 2)
(if Boolean/FALSE 1 2)

clojure.core/=, like Java .equals, returns true when comparing Boolean/FALSE and (Boolean. false). Blame Java.

Comment by Jigen Daisuke [ 05/May/15 4:51 PM ]

@Andy Fingerhut

I know all that, but this doesn't make it any more consistent

See, if they won't fix how "if" treats custom made false-values, the next best
thing to do were to manifest this behaviour of "if" into the "=" function (and
yes, that would mean that "=" isn't a mere ".equals" call anymore, because
Boolean false-values would have to be treated in a special way). But then at
least we would have a consistent system - though not exactly in the way I'd
prefer.

Therefore, I can't blame Java. If they had implemented "if" right in the first place,
everything were fine. So this one is on Clojure.

Comment by Andy Fingerhut [ 05/May/15 5:50 PM ]

I'd like to withdraw the last 2 sentences of my previous comment. I think Alex's comment is the shortest accurate answer: It was a choice in Clojure's implementation to do this. As it is right now, there are some compiled 'if' expressions that compare the test expression against both nil (Java null) and Java Boolean.FALSE, and allowing (Boolean. false) to also be treated as false would either require comparing against a third value, or calling a method like Boolean.valueOf() before doing the comparison. Perhaps a micro-optimization, but seems to me like a reasonable one, given the recommendations in Java not to use the Boolean constructors.





[CLJ-1717] Compiler casts System properties to String without prior type check Created: 29/Apr/15  Updated: 15/Sep/15  Resolved: 18/May/15

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

Type: Defect Priority: Critical
Reporter: Laurent Petit Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: compiler, regression
Environment:

occurs for any JVM / Operating System. Encountered when using Clojure inside the Equinox OSGi framework (reference OSGi implementation used e.g. by Eclipse IDE)


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

 Description   

The Clojure Compiler loops over all System Properties through their EntrySet.
This interface allows for non-String properties to be found. It is leveraged by the Equinox OSGi framework (reference OSGi implementation used e.g. by the Eclipse IDE).

This means that since this new code has been introduced in the Clojure compiler, the official Clojure Jar cannot be used inside Eclipse.

The problem is that Counterclockwise, the Eclipse-based Clojure IDE, at least, is affected, since it is developed with Clojure code.

The attached patch solves the issue by skipping System Properties key/value pairs whose values aren't Strings.



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

Probably a regression related to CLJ-1274.

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

Yes, CLJ-1274 moved the code from the Compile.java class to the Compiler.java class. The code already had the cast problem, but it was probably not an issue at runtime for CCW when only present in Compile.java

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

It looks like maybe this is considered a bug in Eclipse land (which it probably should be)?
https://bugs.eclipse.org/bugs/show_bug.cgi?id=445122

Comment by Laurent Petit [ 29/Apr/15 3:21 PM ]

I agree that it is an abuse on the side of Eclipse-and of a System properties hole that allows to put non-String values and then browse them through methods not using Generics.

From the last comments, it appears that it has only be released very recently, and for only the last stable version of Eclipse. So I expect CCW users with disparate Eclipse versions installed to have the problem if I do not apply the patch to my custom version of Clojure for some more months.

I can live with that if you reject the issue.

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

What is the status for this issue? I understand that being considered an abuse from the OSGi implementation of the System properties contract, you might not want to add code for dealing with this in Compiler.
On the other hand, it's a small 3-lines check with an explanation of why it's done that way, so...

Anyway, feel free to get rid of it and close it so it doesn't get in the way if you think it's not worth the trouble.

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

We haven't had a chance to discuss it further yet. There is reluctance to change it if it's not necessary.

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

Ticket was declined and marked as resolved but not closed. I'm closing it.

Comment by Mike Rodriguez [ 15/Sep/15 11:11 AM ]

For completeness: I'm just going to mention that this came up for me "in the wild" @ https://github.com/stagemonitor/stagemonitor/issues/99.

Basically, our Clojure library was being integrated into larger Java-centric application. One of the other libraries, "stagemonitor", that was used there was failing due to Clojure's initialization performing String casts on all System Properties key-val pairs.

I do agree that it is a bad practice from these other libraries. However, this Clojure implementation is very sensitive to what the JavaDocs for java.util.Properties calls "compromised" Properties
(@ http://docs.oracle.com/javase/7/docs/api/java/util/Properties.html). Clojure could certainly only place the String casting regulations on properties that it was concerned with. I'm guessing this just isn't considered a high priority to do.

It is going to be a little ugly to have to hack around this though when we do not control the other library. I'm thinking it will have to be something like, remove the bad Properties so that Clojure can initialize, then put them back in so nothing else breaks.





[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-1712] clojure.walk returns a different kv pair Created: 21/Apr/15  Updated: 21/Apr/15  Resolved: 21/Apr/15

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

Type: Defect Priority: Minor
Reporter: James Conroy-Finn Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

In Clojure 1.7, a walk that would result in duplicate keys returns different values. Here is a simple example of the behaviour I'm referring to, that passes with Clojure 1.6, and fails with 1.7:

(ns walk.merge-test
  (:require [clojure.walk :refer [keywordize-keys]]
            [clojure.test :refer :all]))

(deftest test-walk-merge
  (is (= (keywordize-keys {:a 1 "a" 2}) {:a 1})))

I don't know if people are relying on this behaviour in production, and consider it a rather pathological example. Regardless, it would certainly be nice if the behaviour were deterministic.

I've pushed a quick demo up to GitHub to make it easier to verify the behaviour should anyone be interested.

https://github.com/jcf/walk-merge



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

Because maps are unordered, this code should not be expected to have deterministic results.

Changes in which map type is used are the likely cause of this difference, but the prior behavior was not something you should rely on.

Comment by James Conroy-Finn [ 21/Apr/15 9:38 AM ]

Thanks for the update Alex. I really enjoyed your Clojure/West talk, BTW.





[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-1710] count of a range may be incorrect Created: 18/Apr/15  Updated: 19/Apr/15  Resolved: 19/Apr/15

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

Type: Defect Priority: Major
Reporter: Nelson Morris Assignee: Devin Walters
Resolution: Duplicate Votes: 0
Labels: None
Environment:

clojure 1.7.0-beta1


Attachments: Text File CLJ-1709-1710.patch    
Patch: Code and Test

 Description   

Reproduction:

(def range-count-is-ceil-of-length-divided-by-step                                                                                                                        
  (prop/for-all [start gen/int                                                                                                                                            
                 end gen/int                                                                                                                                              
                 step (gen/such-that #(not= % 0)                                                                                                                          
                                     gen/int)]                                                                                                                            
                (= (count (range start end step))                                                                                                                         
                   (max 0 (long (Math/ceil (double (/ (- end start) step))))))))                                                                                          
                                                                                                                                                                          
(tc/quick-check 100 range-count-is-ceil-of-length-divided-by-step)                                                                                                        
;;=> {:result false, :seed 1429397156819, :failing-size 6, :num-tests 7, :fail [-1 -3 -4], :shrunk {:total-nodes-visited 9, :depth 4, :result false, :smallest [0 -1 -2]}\
}                                                                                                                                                                         
                                                                                                                                                                          
                                                                                                                                                                          
(range 0 -1 -2)                                                                                                                                                           
;;=> (0)                                                                                                                                                                  
(count (range 0 -1 -2))                                                                                                                                                   
;;=> 0                                                                                                                                                                    
                                                                                                                                                                          
(tc/quick-check 100 range-count-is-ceil-of-length-divided-by-step)                                                                                                        
;;=> {:result false, :seed 1429397201424, :failing-size 5, :num-tests 6, :fail [3 4 3], :shrunk {:total-nodes-visited 9, :depth 4, :result false, :smallest [0 1 2]}}     
                                                                                                                                                                          
(range 0 1 2)                                                                                                                                                             
;;=> (0)                                                                                                                                                                  
(count (range 0 1 2))                                                                                                                                                     
;;=> 0


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

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

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

Covered by CLJ-1709





[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-1696] Generating BigInt sequences with range Created: 04/Apr/15  Updated: 06/Apr/15  Resolved: 06/Apr/15

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

Type: Enhancement Priority: Minor
Reporter: Paul Roush Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

I'm a relative newcomer to the Clojure language, so this input may cover old ground or be seen as naive. The primary substance is a philosophical question, so the response may be that I'm not thinking about this the "right way".

But, for what it's worth, here is the request.

I found myself in a situation where I wanted 'range' to return a sequence of BigInt's. I naively tried something like (range 5N) and had no luck. Later I found that (range 0N 5) would generate BigInt's rather than Long's.

So my first observation / request is that there are cases where it could be "useful" to be able to generate a sequence of BigInt's with a 1-arity call to range.

The bigger point to me, though, is more a philosophical question of whether the behavior of 'range' is as "consistent" with other core API's as it might be. Obviously, this is very subjective.

My naive thinking was something like this...

(* 2 3N 4) => 24N
(* 2 3 4N) => 24N

etc.

...so passing a BigInt value in as any of the 3 possible args to range would promote the result to a sequence of BigInt.

As I now understand, passing in a BigInt as the 'start' value generates a BigInt sequence, but a BigInt 'end' or 'step' value will generate Long's.

To be precise, this "enhancement request" is to modify 'range' so that passing a BigInt as any one of the 3 possible arguments will yield a sequence of BigInt values.

It might also be viewed as a request to make the documentation more explicit in regard to the current behavior.



 Comments   
Comment by Alex Miller [ 06/Apr/15 8:50 AM ]

This is relatively subtle, but the key is that the values of the range are computed based on start (defaults to 0) and step (default to 1) where 0 and 1 are longs. The end value is used for bounds checking but not for computation. From those pieces of info (which are in the docstring, but the long type of the defaults is implied by the lack of N), plus the default numerics behavior of range, I think it's reasonable to predict how this function will work.

I think that modifying the behavior for the computation based on the type of the end value actually breaks the mental model for me and is harder to understand. This, combined with the infrequency of the use case, leads me to close this enhancement.





[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 mor