<< Back to previous view

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

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

Type: Enhancement Priority: Trivial
Reporter: Gregg Reynolds Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

all


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

 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: 30/Aug/15

Status: Open
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: Unresolved Votes: 0
Labels: compiler

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

 Description   

Compiling

(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)

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

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

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.





[CLJ-1808] map-invert should use transients and reduce-kv instead of reduce Created: 30/Aug/15  Updated: 30/Aug/15

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

Type: Enhancement Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File clj-1808-map-invert-should-use-reduce-kv-and-transient.patch    

 Description   

It's more efficient






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

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

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

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

 Description   

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

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

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

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

Example:

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

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






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

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

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

Attachments: Text File clj-1806-1.patch    

 Description   

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



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

patch w/ described changes





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

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

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

Approval: Triaged

 Description   

Currently :rettag works only for expressions like:

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

or

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

But at runtime those type-hints are resolved to

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

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



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

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





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

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

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

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

 Description   

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

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

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


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

Proposed patch, passes all existing tests

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

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

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

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

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

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

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

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

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

(now ~35% faster than original)





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

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

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

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

 Description   

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

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

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

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

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

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

So using such a thing,

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

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

The attached patch makes this change.



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

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

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

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

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

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

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

1.8.0-master:

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

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

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

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

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





[CLJ-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-1800] Doc that lazy-seq with-meta forces realization Created: 13/Aug/15  Updated: 19/Aug/15

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

Type: Enhancement Priority: Minor
Reporter: Max Penet Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File CLJ-1800-no-realize-v1.patch     Text File CLJ-1800-v2.patch    
Approval: Triaged

 Description   

Applying meta to a lazy-seq causes realization:

(def x (vary-meta (lazy-seq (prn :realized)) assoc :foo :bar))
:realized

This might be surprising, so modify docstring of lazy-seq to mention it.

Patch:



 Comments   
Comment by Alex Miller [ 13/Aug/15 9:02 AM ]

I think it's likely that seq() is called here so that the old LazySeq instance and the new one share the sequence. Otherwise the pre-meta and post-meta versions would be performing the same function calls on the same inputs but would be disconnected, which seems bad.

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

I'm not really sure where this would be documented. Maybe on the http://clojure.org/metadata page?

Comment by Max Penet [ 13/Aug/15 9:18 AM ]

That would make sense yes and on the docstring of lazy-seq as well.

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

I added a sentence to the metadata page and updated the description to be more applicable here to a docstring change.

Comment by Michael Blume [ 13/Aug/15 1:29 PM ]

With this patch, with-meta doesn't realize the seq, but realization still only happens once – would this be an acceptable approach?

Comment by Michael Blume [ 19/Aug/15 4:46 PM ]

Added test





[CLJ-1799] Replace refs in pprint Created: 13/Aug/15  Updated: 14/Aug/15

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

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

Attachments: Text File CLJ-1799.patch    

 Description   

I noticed that pprint uses refs and dosync transactions in a number of places, which seems unlikely to be necessary. It seems like these could be replaced by atoms, or even volatiles, given that printing typically happens in a single thread. Presumably this would improve performance of pprint significantly.



 Comments   
Comment by dennis zhuang [ 14/Aug/15 11:28 AM ]

I develop a patch to fix this issue.I run all the tests in clojure and clojure.data.json, and no one fails.

Use criterium to do a simple benchmark as below:

(use 'criterium.core)
(require '[clojure.data.json :as json])
(bench (json/write-str 
  {:a 1 :b 2 :c (range 10) :d "hello world"
   :e (apply hash-set (range 10))}))

before patch:

Evaluation count : 6180060 in 60 samples of 103001 calls.
             Execution time mean : 10.302604 µs
    Execution time std-deviation : 597.958933 ns
   Execution time lower quantile : 9.631444 µs ( 2.5%)
   Execution time upper quantile : 11.618551 µs (97.5%)
                   Overhead used : 1.724553 ns

After patch:

Evaluation count : 6000900 in 60 samples of 100015 calls.
             Execution time mean : 10.212543 µs
    Execution time std-deviation : 564.874941 ns
   Execution time lower quantile : 9.528383 µs ( 2.5%)
   Execution time upper quantile : 11.334033 µs (97.5%)
                   Overhead used : 1.827143 ns




[CLJ-1798] The RetryEx in LockingTransaction should be static Created: 13/Aug/15  Updated: 13/Aug/15

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

Type: Enhancement Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: STM, performance
Environment:

java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)

Mac OSX 10.10.4


Attachments: PNG File profile.png     Text File static_retryex.patch    
Patch: Code
Approval: Triaged

 Description   

We are using clojure.data.json, and we profiled our project with jprofiler, it shows that there is a hotspot in LockingTransaction.Too many RetryEx instances were created during the profiling.
The retryex instance variable should be static as a class variable to avoid creating when a new STM transaction begins.

The attacments are the profile result screen snapshot and the patch to make the retryex to be static.
I don't do any benchmark right now,but maybe a clojure.data.json performance benchmark will approve the patch works better.



 Comments   
Comment by Alex Miller [ 13/Aug/15 8:04 AM ]

I think the suggestion here is sound, but I have a hard time believing it will make much of a difference. My real question is why pprint is using dosync.

Comment by dennis zhuang [ 13/Aug/15 9:03 AM ]

I found it uses many dosync in writer as below:

https://github.com/clojure/clojure/blob/master/src/clj/clojure/pprint/pretty_writer.clj

It seems that using the dosync to change some mutable states.Maybe they can be rewrote into other forms such as atom,java object/lock etc.
But it's another story.





[CLJ-1797] Mention cljc when require fails Created: 10/Aug/15  Updated: 10/Aug/15

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

Approval: Triaged

 Description   

If you attempt to require a namespace for which the code cannot be located, now that cljc files are considered when locating code, it could be mentioned in the list of files considered on the classpath.

$ java -jar clojure-1.7.0.jar 
Clojure 1.7.0
user=> (require 'foo.bar)
FileNotFoundException Could not locate foo/bar__init.class or foo/bar.clj on classpath.  clojure.lang.RT.load (RT.java:449)

Note: FWIW, ClojureScript has similar error messages, and the order listed in the error message is the order tried when loading. If this were followed, I suspect the text of the error message above would end up looking like:

Could not locate foo/bar__init.class, foo/bar.clj, or foo/bar.cljc on classpath.





[CLJ-1796] Protocol functions fail to find future extensions when assigned to a local or new var Created: 08/Aug/15  Updated: 10/Aug/15

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

Type: Defect Priority: Minor
Reporter: Nathan Marz Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: protocols

Approval: Triaged

 Description   
(defprotocol TestProtocol
  (tester [o]))

(let [t tester]
  (defn another-tester [o]
  	(t o)))

(def another-tester2 tester)

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

(another-tester "A") ;; Error
(another-tester2 "A") ;; Error
(tester "A") ;; Works fine

(let [t tester]
  (defn another-tester [o]
  	(t o)))

(another-tester "A") ;; Works fine

(def another-tester2 tester)

(another-tester2 "A") ;; Works fine

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

(another-tester "A") ;; Works fine
(another-tester 3) ;; Error
(another-tester2 3) ;; Error


 Comments   
Comment by Nathan Marz [ 08/Aug/15 12:47 PM ]

This issue appears to be Clojure specific – I did some testing in CLJS and was unable to reproduce the issue.

Comment by Ghadi Shayban [ 09/Aug/15 9:51 AM ]

Nathan,
Not sure if you tried this, but using:

(def another-handle #'the-protocol-function)
rather than dereffing outright.

Comment by Nathan Marz [ 09/Aug/15 6:25 PM ]

That's a good workaround but it does seem that my test case should work. I ran into this because I was passing around functions dynamically and saving them for later execution – and this issue popped up with protocol methods. Having to pass around protocol methods differently than regular functions doesn't seem right.

Comment by Kevin Downey [ 10/Aug/15 11:21 AM ]

this is a result of the protocol implementation in clojure, protocol extension mutates the vars, once you have taken then value of the var (which happens once for top level forms) you will not see further mutations of the var so no more protocol extension





[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-1794] Sorting vector yields non-indexed ArraySeq Created: 05/Aug/15  Updated: 10/Aug/15

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

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

Attachments: Text File 0001-CLJ-1794-Make-ArraySeqs-implement-Indexed.patch    
Approval: Triaged

 Description   

Sorting a vector gives back an ArraySeq with O(n) gets instead of O(log N) gets. This means it can be more efficient to take a vector, sort, then turn it back into a vector.

Cause: sort works by copying the collection to be sorted into an array, calls Arrays/sort to sort it, and then returns a seq on the sorted array. The seq returned is an ArraySeq and doesn't implement Indexed.

Alternatives:

1. Make ArraySeq (and primitive specializations thereof) implement Indexed, providing constant time lookup by index.
2. Specialize sorting for different collection types
3. ???



 Comments   
Comment by Ragnar Dahlen [ 06/Aug/15 2:28 AM ]

Update description, attach patch.

Comment by Ragnar Dahlen [ 06/Aug/15 2:31 AM ]

Added link to current patch.

Comment by Alex Miller [ 06/Aug/15 6:50 AM ]

Another alternative to consider here is to have sort do something smarter.

Comment by Ragnar Dahlen [ 06/Aug/15 7:44 AM ]

Having thought a bit more about the approach and implications of this I'm not sure this patch is a good idea at all. It makes a little bit sense for the particular case of sorting a vector, but on the other hand sort only promises to return a sorted sequence of given coll. Implementing Indexed for a sequence type just because the underlying data structure supports efficient lookup by index feels wrong. Like you suggest, effort is maybe better spent thinking about making sort smarter, which is a different issue, or just using sorted collections instead.

Comment by Kevin Downey [ 06/Aug/15 12:49 PM ]

It seems like the best thing here would be to change sort to return a vector. Usages of sort in the middle of sequence pipelines will continue to work, but a sort followed by conj will break (I cannot recall an instance of this off hand, but I am sure they exist). Sorting seems to imply a fully realized collection, and vectors are the "strongest" realized collections that can be returned here.

Given the conservative nature of core, and the issue with conj ordering above, the next best thing might be to add a sortv similar to the existing mapv.

Another option might be to remove the call to seq, so sort returns the sorted array. This would actually be really useful because you can use Arrays.binarySearch. Calls to conj after a sort would then fail with an exception instead of conj to the "wrong" place.





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

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

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

1.8.0-alpha2 - 1.8.0-alpha4


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

 Description   

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

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

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

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

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

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

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

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

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

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

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

Patch: clj-1793-2.patch

Screened by: Alex Miller



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

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

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

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

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

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

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

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

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

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

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

See the commit message for an extensive-ish comment.

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

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

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

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

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

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

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

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





[CLJ-1792] array-map and hash-map differ in handling of keys comparing identical but not equal Created: 04/Aug/15  Updated: 04/Aug/15

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

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

Attachments: Text File 0001-do-pointer-check-in-Util.EquivPred.patch    
Patch: Code

 Description   
user=> (let [a (array-map Double/NaN 1)] (assoc a (key (first a)) "foo"))
{NaN 1, NaN "foo"}
user=> (let [a (hash-map Double/NaN 1)] (assoc a (key (first a)) "foo"))
{NaN "foo"}

Approach: Array-map's comparison skips identity-check and always delegates to .equals calls. The attached patch alignes array-map behaviour with hash-map by doing the appropriate pointer checks before delegating to .equals






[CLJ-1791] Issue defining a defrecord protocol method named "clear" Created: 04/Aug/15  Updated: 08/Aug/15

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

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


 Description   

There seems to be a problem in trying to define a protocol with a method named "clear"

(defprotocol PClear
(clear [o]))
=> PClear

(defrecord Foo []
PClear
(clear [o] o))
=> CompilerException java.lang.ClassFormatError: Duplicate method name&signature in class file xxxx/Foo, compiling:(NO_SOURCE_PATH:1:1)

I assume this is due to a name conflict with the Java method Collection.clear() in the underlying implementation. However the error is very unclear about this, and the potential for conflict appears to be undocumented as far as I can see.

There seem to be two possible approaches to fixing this:
a) Disallow the use of "clear" as a protocol method name (in which case the error should be more informative, and the rule should be documented)
b) Find a way to support this in the class file format (possibly by overloading on JVM return types, since Collection.clear() returns void??)



 Comments   
Comment by Nicola Mometto [ 04/Aug/15 6:58 AM ]

Mike, the jvm doesn't support return type overloading so your second suggestion is not technically possible.

Reading the doc for defrecord " The class will have implementations of several (clojure.lang)
interfaces generated automatically: IObj (metadata support) and
IPersistentMap, and all of their superinterfaces.
"

Perharps java.util.Collection (or even better, java.util.Map) should be mentioned here.

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

I think this should be a doc enhancement request.

Comment by OHTA Shogo [ 08/Aug/15 3:42 AM ]

It might be out of the scope of this ticket, but protocol method conflicts can cause some other kinds of errors:

user=> (defprotocol P1 (finalize [this]))
P1
user=> (defrecord R1 [] P1 (finalize [this]))

CompilerException java.lang.VerifyError: (class: user/R1, method: finalize signature: ()Ljava/lang/Object;) Unable to pop operand off an empty stack, compiling: ...
user=> (defprotocol P2 (wait [this]))
P2
user=> (defrecord R2 [] P2 (wait [this]))
user.R2
user=> (def r (->R2))
#'user/r
user=> (wait r)
CompilerException java.lang.IllegalArgumentException: No single method: wait of interface: user.P2 found for function: wait of protocol: P2, compiling: ...
user=>

IMHO it would be nicer if defprotocol would warn method conflicts with a more informative message.





[CLJ-1790] Problem with 1.8.0-alpha3 handling of protocols with Java arrays Created: 29/Jul/15  Updated: 30/Jul/15

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

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


 Description   

1.8.0-alpha3 is breaking core.matrix at the moment.

Root cause appears to be related to how protocols are being handled when used with Java arrays:

e.g. for the protocol implementation:

(extend-protocol mp/PImplementation
  (Class/forName "[Ljava.lang.Object;")
    (implementation-key [m] :object-array)
    (meta-info [m]
      {:doc "Clojure.core.matrix implementation for Java Object arrays"})
    (new-vector [m length] (construct-object-vector (long length)))
    (new-matrix [m rows columns]
      (let [columns (long columns)
            m (object-array rows)]
        (dotimes [i rows]
          (aset m i (construct-object-vector columns)))
        m))
    (new-matrix-nd [m shape]
      (construct-nd shape))
    (construct-matrix [m data]
      (construct-object-array data))
    (supports-dimensionality? [m dims]
      (>= dims 1)))

When called as:

(clojure.core.matrix.protocols/construct-matrix (object-array 1) [1])

Gives exception:

VerifyError (class: clojure/core/matrix$eval10586, method: invokeStatic signature: ()Ljava/lang/Object;) Incompatible object argument for function call  java.lang.Class.getDeclaredConstructors0 (:-2)





[CLJ-1789] Use transients with select-keys if possible Created: 28/Jul/15  Updated: 28/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance


 Description   

Currently select-keys uses conj to add entries. If the map is editable, conj! could be used instead to improve select-keys performance.

Additionally keyseq is traversed as a seq but could be traversed via reduce instead, which might be faster.






[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-1787] Add executables to distribution (suggested changes included) Created: 27/Jul/15  Updated: 27/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Morten Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Linix, OS X and Windows


Patch: Code

 Description   

Being new to clojure I was surprised that there was not a executable included in the download. It makes Clojure appear less proff then it is and is unnecessary complex to remember the exact java cmd line to use so I suggest that shell executable files are added to the distibution for the next version of Clojure. They could look as suggest below for Windows and OS X:

1) Linux and OS X:
#!/bin/bash
java -jar `dirname "$0"`/clojure-1.8.0.jar $@

2) Windows:
java -jar %~dp0\clojure-1.8.0.jar %1 %2 %3 %4 %5 %6 %7 %8






[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-1784] Reflector.getMethods should be cached Created: 21/Jul/15  Updated: 28/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Vladimir Sitnikov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Currently Reflector.getMethods performs expensive logic that includes java.lang.reflect.Method copying.
See: https://github.com/clojure/clojure/blob/b8607d5870202034679cda50ec390426827ff692/src/jvm/clojure/lang/Reflector.java#L373

In our application I see the following back-traces:

at Reflector.copyMethods
at Reflector.invokeInstanceMethod
at ...

These kind of backtraces are second top consumers of all the heap allocation.

JDK cannot cache Methods / Fields since they are mutable (e.g. user can call setAccessible here and there).
However, for the purposes of Clojure, I believe it should be fine to cache Methods and Fields.

What do you think?
E.g. WeakHashMap<Class, WeakReference<List<Method>>> or more sophisticated structure to account String name.



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

If you are seeing Reflector as a hot spot in your application, you should probably turn on warn-on-reflection and use type hints to avoid reflection.

Comment by Vladimir Sitnikov [ 28/Jul/15 6:10 AM ]

Do you mean there is absolutely no reason to use reflection in Clojure ever?
I do understand that if developer gives enough type hints the reflection would go away.

However:
1) I just do not know if it is easily doable (in other words, if it is possible at all, maintainable, etc)
2) I'm not sure if "always use type hints" is considered a best practice. For instance, warn-on-reflection documentation page says nothing like "always use type hints"
3) Caching copyMethods seems to be a low-hanging fruit here, so it would shave cpu cycles for those who omitted type hints

PS. I'm a java performance engineer, not a Clojure engineer (as in "my Clojure knowledge is somewhere near (+ x y)"), so I kindly beg on your forgiveness for me not doing RTFM.

Comment by Alex Miller [ 28/Jul/15 7:44 AM ]

No, I'm saying that if reflection is a hotspot in your application, usually it's worth investing a few minutes to add type hints in those hotspot areas and this is common advice for Clojure apps. Once that minimal work is done, few Clojure apps are bound by reflection.

Caching seems like an easy solution until you consider all of the management aspects. How does the cache get cleaned? Are the instances mutable and able to be reused? Are there cases where class loaders or code reloading create unexpected side effects? What are the concurrency effects of putting a shared resource in the invocation path? What is the memory impact of a cache and is it configurable?

Those are all things that would need to be investigated, meaning that this is not low-hanging fruit.





[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-1782] Hide local IDE files in .gitignore Created: 21/Jul/15  Updated: 21/Jul/15

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

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

Attachments: Text File clj-1782.patch    

 Description   

Several IDEs (e.g. Eclipse/CCW, IntelliJ IDEA/Cursive) create local files in project workspaces which should normally be ignored for the purposes of source control.

This patch proposes to add some common files that should be ignored to the .gitignore






[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-1779] Optimise compiler usage of getMethod calls Created: 17/Jul/15  Updated: 17/Jul/15

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

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


 Description   

Currently the Clojure compiler makes multiple redundant calls to Method.getMethod(...) while emitting code, e.g.

gen.invokeStatic(Type.getType(Long.class), Method.getMethod("Long valueOf(long)"));

It seems to be the case that that:

a) These getMethod calls are effectively returning equivalent, immutable constant values
b) getMethod is moderately expensive (performs string analysis and quite a few object allocations)
c) These calls are very common during compilation of typical Clojure code

The proposed enhancement is to replace all of these getMethod calls with constant static values. This should improve compilation performance noticeably with no effect on behaviour.






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

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

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

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

 Description   

Seen in a tweet...

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

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

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

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

Screened by: Alex Miller



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

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

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

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

Updated patch with two changes:

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

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

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

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

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

Is this the smallest change that can be made?

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

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

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

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

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

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

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

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

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

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

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

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

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

The current patch makes the following changes:

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

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

Please excuse the rather lengthy reply.

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

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

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





[CLJ-1777] Add function version of vswap! Created: 13/Jul/15  Updated: 14/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File 0001-CLJ-1777-Change-vswap-from-a-macro-to-an-inlineable-.patch    
Patch: Code

 Description   

Volatiles and vswap! were added in 1.7 as performant mechanisms to achieve uncoordinated mutation to the language.
given the fact that their addition was a performance-centric one, vswap! was implemented as a macro rather than a normal function to avoid runtime dereference of the function var and the optional apply overhead in case of multiple args.

However this:
-is not necessary
-breaks the api parallelism between volatile/atom swap!/vswap! reset!/vreset!
-makes impossible certain use cases (vswaps! in update-in forms)
-is potentially confusing given that swap! is a function

Infact the macro can be replaced with a function with :inline metadata.
This is a strictly additive change that will make so that for all the current valid usages of vswap! nothing will change, it will still be macroexpanded by the inliner and additionally since it is now a function it can be used in HOF contexts where it's not unusual to see swap! used.



 Comments   
Comment by Alex Miller [ 13/Jul/15 8:07 PM ]

Nicola, please don't set the fix version on tickets.

Comment by Nicola Mometto [ 13/Jul/15 8:10 PM ]

Sorry, I mixed the fix version with the affected field





[CLJ-1776] Test that collections are valid statements Created: 08/Jul/15  Updated: 08/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Michael Blume Assignee: Michael Blume
Resolution: Unresolved Votes: 0
Labels: compiler, test

Attachments: Text File clj-1776-v1.patch    
Patch: Code and Test

 Description   

It's possible to break the compiler such that vectors are emitted incorrectly when they're in statement position (I accidentally did this). This doesn't break any part of the Clojure test suite, but does break valid Clojure code (for me it hit taoensso's encore). Add tests to the test suite so defects of this kind are caught.






[CLJ-1775] Add any? Created: 07/Jul/15  Updated: 09/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Joshua Griffith Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Given the presence of `every?` and `not-every?`, it seems that `any?` should also exist given `not-any?`. While similar to `some`, indeed `(def any? (comp boolean some))`, it would make the interface consistent. Also, having `any?` would be nice when writing typed Clojure, where one often wants a boolean.



 Comments   
Comment by Colin Taylor [ 09/Jul/15 5:34 PM ]

Previous discussion - https://groups.google.com/d/msg/clojure/02msnrXJsSg/BdgfoeyVX7sJ.
My bad example notwithstanding, I still think this is symmetric and useful for interop. The typed Clojure point is valid too..





[CLJ-1774] Field access on typed record does not preserve type Created: 02/Jul/15  Updated: 03/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: defrecord, reflection, typehints


 Description   
(ns field-test.core
  (:import [java.util UUID]))

(defrecord UUIDWrapper [^UUID uuid])

(defn unwrap [^UUIDWrapper w]
  (.-uuid w)) ; <- No reflection

(defn get-lower-bits [^UUIDWrapper w]
  (-> w .-uuid .getLeastSignificantBits)) ; <- Reflection :(

The compiler seems to have all the information it needs, but lein check prints

Reflection warning, field_test/core.clj:10:3 - reference to field getLeastSignificantBits on java.lang.Object can't be resolved.

(test case also at https://github.com/MichaelBlume/field-test)



 Comments   
Comment by Alex Miller [ 02/Jul/15 4:31 PM ]

afaik, that ^UUID type hint on the record field doesn't do anything. The record field will be of type Object (only ^double and ^long affect the actual field type).

Perhaps more importantly, it is bad form to use Java interop to retrieve the field values of a record. Keyword access for that is preferred.

Comment by Nicola Mometto [ 03/Jul/15 4:48 AM ]

The same issue applies for deftypes where keyword access is not an option

Comment by Alex Miller [ 03/Jul/15 12:17 PM ]

Per http://clojure.org/datatypes: "You should always program to protocols or interfaces -
datatypes cannot expose methods not in their protocols or interfaces"

Along those lines, usually for deftypes, I gen an interface with the proper types if necessary, then have the deftype implement the interface to expose the field.

Also per http://clojure.org/datatypes:

"note that currently a type hint of a non-primitive type will not be used to constrain the field type nor the constructor arg, but will be used to optimize its use in the class methods" - that is, inside a method implemented on the record/type, referring to the field should have the right hint. So in the example above, if unwrap was an interface or protocol implementation method on the record, and you referred to the field, you should expect the hint to be utilized in that scenario.

So, my contention would be that all of the behavior described in this ticket should be expected based on the design, which is why I've reclassified this as an enhancement, not a defect.





[CLJ-1773] Support for REPL commands for tooling Created: 01/Jul/15  Updated: 11/Aug/15  Resolved: 11/Aug/15

Status: Resolved
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).





[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-1771] Support for multiple key(s)-value pairs in assoc-in Created: 29/Jun/15  Updated: 23/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Griffin Smith Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

All


Attachments: Text File clj-1771.patch    
Approval: Triaged

 Description   

It would be nice if assoc-in supported multiple key(s)-to-value pairs (and threw an error when there were an even number of arguments, just like assoc):

user=> (assoc-in {} [:a :b] 1 [:c :d] 2)
{:a {:b 1}, :c {:d 2}}
user=> (assoc-in {} [:a :b] 1 [:c :d])
IllegalArgumentException assoc-in expects even number of arguments after map/vector, found odd number


 Comments   
Comment by Matthew Gilliard [ 23/Jul/15 2:15 PM ]

Simple patch attached. I did not find any existing tests for assoc-in but I could add them if wanted.





[CLJ-1770] atom watchers are not atomic with respect to reset! Created: 29/Jun/15  Updated: 31/Jul/15

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

Type: Defect Priority: Major
Reporter: Eric Normand Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: atom

Attachments: Text File atom-reset-atomic-watch-2015-06-30.patch     File timingtest.clj    
Patch: Code and Test
Approval: Triaged

 Description   

It is possible that two threads calling `reset!` on an atom can interleave, causing the corresponding watches to be called with the same old value but different new values. This contradicts the guarantee that atoms update atomically.

(defn reset-test []
  (let [my-atom (atom :start
                      :validator (fn [x] (Thread/sleep 100) true))
        watch-results (atom [])]
    (add-watch my-atom :watcher (fn [k a o n] (swap! watch-results conj [o n])))
  
    (future (reset! my-atom :next))
    (future (reset! my-atom :next))
    (Thread/sleep 500)
    @watch-results))

(reset-test)

Yields [[:start :next] [:start :next]]. Similar behavior can be observed when mixing reset! and swap!.

Expected behavior

Under atomic circumstances, (reset-test) should yield [[:start :next] [:next :next]]. This would "serialize" the resets and give more accurate information to the watches. This is the same behavior one would achieve by using (swap! my-atom (constantly :next)).

(defn swap-test []
  (let [my-atom (atom :start
                      :validator (fn [x] (Thread/sleep 100) true))
        watch-results (atom [])]
    (add-watch my-atom :watcher (fn [k a o n] (swap! watch-results conj [o n])))
  
    (future (swap! my-atom (constantly :next)))
    (future (swap! my-atom (constantly :next)))
    (Thread/sleep 500)
    @watch-results))

(swap-test)

Yields [[:start :next] [:next :next]]. The principle of least surprise suggests that these two functions should yield similar output.

Alternative expected behavior

It could be that atoms and reset! do not guarantee serialized updates with respect to calls to watches. In this case, it would be prudent to note this in the docstring for atom.

Analysis

The code for Atom.reset non-atomically reads and sets the internal AtomicReference. This allows for multiple threads to interleave the gets and sets, resulting in holding a stale value when notifying watches. Note that this should not affect the new value, just the old value.

Approach

Inside Atom.reset(), validation should happen first, then a loop calling compareAndSet on the internal state (similar to how it is implemented in swap()) should run until compareAndSet returns true. Note that this is still faster than the swap! constantly pattern shown above, since it only validates once and the tighter loop should have fewer interleavings. But it has the same watch behavior.

public Object reset(Object newval){
    validate(newval);
    for(;;)
        {
            Object oldval = state.get();
            if(state.compareAndSet(oldval, newval))
                {
                    notifyWatches(oldval, newval);
                    return newval;
                }
        }
}


 Comments   
Comment by Eric Normand [ 30/Jun/15 9:24 AM ]

I've made a test just to back up the timing claims I made above. If you run the file timingtest.clj, it will run code with both reset! and swap! constantly, with a validator that sleeps for 10ms. In both cases, it will print out the number of uniques (should be equal to number of reset!s, in this case 1000) and the time (using clojure.core/time). The timing numbers are relative to the machine, so should not be taken as absolutes. Instead, the ratio between them is what's important.

Run with: java -cp clojure-1.7.0-master-SNAPSHOT.jar clojure.main timingtest.clj

Results

Existing implementation:

"Elapsed time: 1265.228 msecs"
Uniques with reset!: 140
"Elapsed time: 11609.686 msecs"
Uniques with swap!: 1000
"Elapsed time: 7010.132 msecs"
Uniques with swap! and reset!: 628

Note that the behaviors differ: swap! serializes the watchers, reset! does not (# of uniques).

Suggested implementation:

"Elapsed time: 1268.778 msecs"
Uniques with reset!: 1000
"Elapsed time: 11716.678 msecs"
Uniques with swap!: 1000
"Elapsed time: 7015.994 msecs"
Uniques with swap! and reset!: 1000

Same tests being run. This time, they both serialize watchers. Also, the timing has not changed significantly.

Comment by Eric Normand [ 30/Jun/15 10:16 AM ]

Adding atom-reset-atomic-watch-2015-06-30.patch. Includes test and implementation.





[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-1768] quote of an empty lazyseq produces an error when evaled Created: 24/Jun/15  Updated: 24/Jun/15

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

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

Approval: Triaged

 Description   
user=> (eval `'())
()
user=> `'~(map identity ())
(quote ())
user=> (eval `'~(map identity ()))    ;; expected: ()
CompilerException java.lang.UnsupportedOperationException: Unknown Collection type, compiling:(NO_SOURCE_PATH:5:1)
user=> (prn *e)
#error {
 :cause "Unknown Collection type"
 :via
 [{:type clojure.lang.Compiler$CompilerException
   :message "java.lang.UnsupportedOperationException: Unknown Collection type, compiling:(NO_SOURCE_PATH:5:1)"
   :at [clojure.lang.Compiler analyzeSeq "Compiler.java" 6730]}
  {:type java.lang.UnsupportedOperationException
   :message "Unknown Collection type"
   :at [clojure.lang.Compiler$EmptyExpr emit "Compiler.java" 2929]}]
 :trace
 [[clojure.lang.Compiler$EmptyExpr emit "Compiler.java" 2929]
  [clojure.lang.Compiler$BodyExpr emit "Compiler.java" 5905]
  [clojure.lang.Compiler$FnMethod doEmit "Compiler.java" 5453]
  [clojure.lang.Compiler$FnMethod emit "Compiler.java" 5311]
  [clojure.lang.Compiler$FnExpr emitMethods "Compiler.java" 3843]
  [clojure.lang.Compiler$ObjExpr compile "Compiler.java" 4489]
  [clojure.lang.Compiler$FnExpr parse "Compiler.java" 3983]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6721]
  [clojure.lang.Compiler analyze "Compiler.java" 6524]
  [clojure.lang.Compiler eval "Compiler.java" 6779]
  [clojure.lang.Compiler eval "Compiler.java" 6745]
  [clojure.core$eval invoke "core.clj" 3081]
  ;; elided rest
nil
user=> (eval `'~(map identity '(x)))
(x)

Cause: In the empty list case, the compiler here sees a LazySeq. I suspect something earlier in the stack should be producing an empty list instead, but haven't tracked it back yet.






[CLJ-1767] Documentation Issues with sort Created: 22/Jun/15  Updated: 22/Jun/15

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

Type: Defect Priority: Minor
Reporter: Marc O'Morain Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring


 Description   

The documentation for sort seems to be incomplete:

  • sort can return nil in some situations. There is a discussion in CTYP-228 with more backstory. There is a repro case here: http://sprunge.us/VIFc?clj supplied by Nicola Mometto. The doc string states that sort "Returns a sorted sequence of the items in coll.", which does not indicate that sort can return nil.
  • It is stated that the "comparator must implement java.util.Comparator.", but this is not true - the comparator can be any IFn that can accept 2 arguments and return either a Boolean or a Number.

For the first issue (nil return), changing the implementation to never return nil might be the neatest fix. For the second issue, the docstring could reference a description of how what functions are valid comparison functions, which can be referenced by other functions that also accept comparators (e.g., sort-by, sorted-set-by, sorted-map-by).



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

The first issue is being covered by CLJ-1763, so I would remove it from the ticket.

The second is technically true - Arrays.sort() will invoked and it takes a Comparator. The tricky bit is that AFn base class implements Comparator so all function implementations that extend from it that support a 2-arg function satisfy this constraint. But it might be helpful to call that out better.





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

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

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

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


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

 Description   

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

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

Examples:

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

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

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

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

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

Alternate approaches:

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

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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





[CLJ-1765] areduce speed improvements Created: 19/Jun/15  Updated: 20/Jun/15

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

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

OSX 10.8.5, Oracle JDK1.8.0_25-b17


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

 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.patch
Screened by: Alex Miller



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

added patch w/ changes described above





[CLJ-1764] partition-by runs infinite loop when one element of infinite partition is accessed Created: 19/Jun/15  Updated: 23/Jul/15

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

Type: Defect Priority: Minor
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: newbie

Attachments: Text File clj-1764.patch    
Approval: Triaged

 Description   
(def x (partition-by zero? (range)))
(first x)
;-> (0)
(first (second x))
;-> infinite loop

The reason is that partition-by counts and thus realizes each current partition to call itself recursively.

It seems like unexpected behavior, since the user may not intend to realize the entire partition.

It can be fixed by changing seq to lazy-seq in its last line.



 Comments   
Comment by Matthew Gilliard [ 23/Jul/15 2:36 PM ]

Patch as suggested by Leon, + test.





[CLJ-1763] clojure.core/sort is not thread-safe on Java collections with backing arrays Created: 19/Jun/15  Updated: 30/Jul/15

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

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

Attachments: Text File 0001-CLJ-1763-make-sort-thread-safe.patch    
Patch: Code
Approval: Triaged

 Description   

If a (mutable) Java collection that exposes it's backing array is passed to c.c/sort in multiple threads, the collection will be concurrently modified in multiple threads.

user=> (def q (java.util.concurrent.ArrayBlockingQueue. 1))
#'user/q
user=> (future (loop [] (.add q 1) (.remove q 1) (recur)))
#object[clojure.core$future_call$reify__4393 0x4769b07b {:status :pending, :val nil}]
user=> (take 3 (distinct (repeatedly #(sort q))))
((1) () nil)

Approach: Convert coll to a seq before converting it to an array, thus preserving the original collection.

Patch: 0001-CLJ-1763-make-sort-thread-safe.patch

Alternate approaches:

1. Document in sort that, like Java arrays, Java collections backed by arrays are modified in-place.
2. Change RT.toArray() to defensively copy the array returned from a (non-IPersistentCollection) Java collection. This has a number of potential ramifications as this method is called from several paths.
3. For non-Clojure collections, could also use Collections.sort() instead of dumping to array and using Arrays.sort().



 Comments   
Comment by Alex Miller [ 19/Jun/15 10:55 AM ]

The docstring says "If coll is a Java array, it will be modified. To avoid this, sort a copy of the array." which also seems like solid advice in this case.

Creating a sequence view of the input collection would significantly alter the performance characteristics.

Comment by Alex Miller [ 19/Jun/15 10:59 AM ]

I guess what I'm saying is, we should not make the performance worse for persistent collections in order to make it safer for arbitrary Java collections. But it may still be useful to make it safer without affecting persistent collection performance and/or updating the docstring.

Comment by Nicola Mometto [ 19/Jun/15 11:02 AM ]

Alex, no additional sequence is being created, the seq call was already there

Comment by Alex Miller [ 19/Jun/15 11:53 AM ]

Well, that's kind of true. The former use did not force realization of the whole seq, just the first element. That said, from a quick test the extra cost seems small on a set (vector seqs are actually faster due to their structure).

Comment by Stuart Halloway [ 30/Jul/15 9:11 AM ]

I think this should be a docstring change, if anything at all.





[CLJ-1762] Implement IKVReduce for java.util.map Created: 18/Jun/15  Updated: 18/Jun/15

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

Type: Enhancement Priority: Minor
Reporter: Chen Guo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop, performance

Attachments: Text File reduce-kv-java-map.patch    
Patch: Code
Approval: Triaged

 Description   

reduce works on Java maps, but reduce-kv does not:

user=> (def clojure-map {1 2 3 4})
#'user/clojure-map
user=> (def java-map (java.util.HashMap. {1 2 3 4}))
#'user/java-map
user=> (reduce (fn [sum [k v]] (+ sum k v)) 0 java-map)
10
user=> (reduce-kv + 0 clojure-map)
10
user=> (reduce-kv + 0 java-map)

IllegalArgumentException No implementation of method: :kv-reduce of protocol: #'clojure.core.protocols/IKVReduce found for class: java.util.HashMap  clojure.core/-cache-protocol-fn\
 (core_deftype.clj:544)

It's trivial to destructure arguments in a regular reduce, but there are performance implications. The following example yields a 7x speed up when run with the implementation of reduce-kv for java.util.Map as implemented in this patch:

user=> (def big-clojure-map (into {} (map #(vector % %) (range 10000))))
#'user/big-clojure-map
user=> (def big-java-map (java.util.HashMap. big-clojure-map))
Reflection warning, /tmp/form-init7130245387362554027.clj:1:19 - call to java.util.HashMap ctor can't be resolved.
#'user/big-java-map
user=> (defn reduce-sum [m] (reduce (fn [sum [k v]] (+ sum k v)) 0 m))
#'user/reduce-sum
user=> (defn reduce-sum-kv [m] (reduce-kv (fn [sum k v] (+ sum k v)) 0 m))
#'user/reduce-sum-kv
user=> (time (dotimes [_ 1000] (reduce-sum big-java-map)))
"Elapsed time: 2624.692113 msecs"
nil
user=> (time (dotimes [_ 1000] (reduce-sum-kv big-java-map)))
"Elapsed time: 376.802454 msecs"
nil





[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: 27/Jun/15

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

Type: Enhancement Priority: Major
Reporter: Mario T. Lanza Assignee: Unassigned
Resolution: Unresolved Votes: 0
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





[CLJ-1759] macroexpand throws runtime exception on symbol bound to a class Created: 17/Jun/15  Updated: 18/Jun/15

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

Type: Defect Priority: Minor
Reporter: W. David Jarvis Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler
Environment:

OSX 10.10.3, Leiningen 2.5.1, Java 1.8.0_45 64-bit.


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

 Description   

The use of macroexpand on short class name symbols triggers a RuntimeException.

user=> (import 'java.net.URI)
java.net.URI
user=> (macroexpand '(java.net.URI "http://google.com")) ;; fine
(java.net.URI "http://google.com")
user=> (macroexpand '(URI "http://google.com")) ;; huh?
java.lang.RuntimeException: Expecting var, but URI is mapped to class java.net.URI
user=> (pst *e)
RuntimeException Expecting var, but URI is mapped to class java.net.URI
	clojure.lang.Util.runtimeException (Util.java:221)
	clojure.lang.Compiler.lookupVar (Compiler.java:7092)
	clojure.lang.Compiler.isMacro (Compiler.java:6571)
	clojure.lang.Compiler.macroexpand1 (Compiler.java:6626)
	clojure.core/macroexpand-1 (core.clj:3870)
	clojure.core/macroexpand (core.clj:3879)

Neither of these should throw an error during macroexpansion (basically should be same after expansion. Both should throw the same error when evaluated (ClassCast trying to invoke a Class as an IFn).

Approach: Throw the runtime error in lookupVar only if internNew is true. In that case we unexpectedly found something other than a var and should still report. Otherwise, just let lookupVar flow through to return a null (no var found.



 Comments   
Comment by Alex Miller [ 18/Jun/15 6:19 AM ]

The compiler is trying to determine if the thing in function position is a var that is a macro that requires expansion in Compiler.isMacro().

In the case of (java.net.URI "http://google.com"), lookupVar determines that java.net.URI is an unmapped symbol and does nothing, meaning no expansion is necessary (this of course will fail at evaluation time with "ClassCastException java.lang.Class cannot be cast to clojure.lang.IFn").

In the case of (URI "http://google.com"), lookupVar finds a symbol mapped to something that's not a var and throws the RuntimeException that is seen.

I would expect that neither of these should throw an error during macroexpansion (basically the same thing they start as) and that both should throw the same error when evaluated. Attaching a patch that will only throw the error if internNew - in that case you unexpectedly found something other than a var and you should still report (otherwise, just return null - lookupVar didn't find a var).

The internNew case comes up with:

(import java.net.URI) 
(def URI "abc") ;; java.lang.RuntimeException: Expecting var, but URI is mapped to class java.net.URI

with the patch:

user=> (macroexpand '(java.net.URI "http://google.com")) 
(java.net.URI "http://google.com") 
user=> (macroexpand '(URI "http://google.com")) 
(URI "http://google.com") 
user=> (java.net.URI "http://google.com") 
ClassCastException java.lang.Class cannot be cast to clojure.lang.IFn user/eval9 (NO_SOURCE_FILE:6) 
user=> (URI "http://google.com") 
ClassCastException java.lang.Class cannot be cast to clojure.lang.IFn user/eval11 (NO_SOURCE_FILE:7)




[CLJ-1758] xf overload for vec and set Created: 17/Jun/15  Updated: 17/Jun/15

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

Type: Enhancement Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Having (vec xf coll) and (set xf coll) overloads seem useful as opposed to writing (into [] ...).

One might also consider these as variadic overloads, like the sequence function has. I am unsure about that since into doesn't have one and I know too little about multiple input transducers.






[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: 16/Jun/15

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

Type: Defect Priority: Minor
Reporter: Patrick Gombert Assignee: Unassigned
Resolution: Unresolved 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?





[CLJ-1754] Destructuring with :merge Created: 16/Jun/15  Updated: 16/Jun/15

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

Type: Enhancement Priority: Major
Reporter: Henrik Heine Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Destructuring with :or {...} does not add those defaults to :as binding. Destructuring with :merge adds this.
Usefull when you're wrapping calls to functions and want to add defaults that your callers need not pass in.

(defn foo [& {:merge {:c "C" :d "D"}
:as opt-args}]
opt-args)

should behave like:

(defn foo [& {:keys [c d]
:or {c "C" d "D"}
:as opt-args}]
(let [opt-args (merge {:c c :d d} opt-args)]
opt-args))

Options:
(a) the bindings for c and d in the example may be usefull or not. For the :merge example above they are not needed.
(b) :merge could use keywords or symbols. keywords make it look like the (merge) and symbols make it look like :keys/:or.

Suggestion: using symbols will build a binding for those names and using keywords will not. So users can get the bindings if they need them.

see also https://groups.google.com/forum/#!folder/Clojure$20Stuff/clojure/gG6Tzssn9Nw



 Comments   
Comment by Alex Miller [ 16/Jun/15 7:14 AM ]

Destructuring is about extracting parts of a composite input value. This seems to go a step beyond that into transformation of the input value. Can't say I am a fan of that but I will leave it open.





[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-1752] realized? return true for an instance that is not IPending Created: 09/Jun/15  Updated: 09/Jun/15

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

Type: Enhancement Priority: Minor
Reporter: Logan Linn Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Triaged

 Description   

To safely test if an arbitrary seq is realized (non-lazy), we need a wrapper like:

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

If realized? returned true for an (ISeq?) instance that is not IPending there would be less surprising behavior for cases such as, (realized? (range 10)) which throws exception.

NB: A follow-up to CLJ-1751.






[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-1750] There should be a way to observe platform features at runtime Created: 08/Jun/15  Updated: 30/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader

Approval: Triaged

 Description   

Reader conditionals let the reader emit code conditionally based upon a set of platform features.

This is a closed set - however, currently it is baked in as an implementation detail of the reader. Runtime code cannot access the current platform feature set.

This is problematic when writing a macro that needs to emit code conditionally based upon the platform of the code being compiled. Reader conditionals themselves won't work since macros are always themselves read in Clojure.

We should enable some mechanism for retrieving the current platform at runtime, or at least at macro expansion time.

For example, this is the kind of thing it should be possible to do:

(defmacro mymacro []
    (if (*platforms* :clj)
      `(some-clojure-thing)
      `(some-cljs-thing)))


 Comments   
Comment by Micah Martin [ 19/Jun/15 1:46 PM ]

+1 - Would very much like to see this in 1.7. Currently I have to use an ugly hack.

(def ^:private ^:no-doc cljs? (boolean (find-ns 'cljs.analyzer)))





[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-1748] Change clojure.core/reverse to return rseq for args that are Reversible Created: 07/Jun/15  Updated: 07/Jun/15

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

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


 Description   

There may be issues with this suggestion about concrete types of return values, or doc strings that promise things that you want to preserve that cannot be preserved with this suggested change.

However, if those issues are not show stoppers, changing clojure.core/reverse to check if its arg is Reversible, and if so, return rseq, else do as reverse does today, could be faster in many situations.






[CLJ-1747] Eduction's print-method expects its collection to be Iterable/Sequential Created: 07/Jun/15  Updated: 07/Jun/15

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

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

Attachments: Text File CLJ-1747-eduction-print.patch    
Patch: Code

 Description   

eduction expects its source collection to be Iterable [1], and its print-method goes through print-sequential [2]. This implies a promise that may restrict the use-case of an eduction over a virtual collection, e.g. an IReduceInit source that may be backed by I/O or some other resource. I have found it useful to construct these I/O reducibles and wrap them with an eduction. If you are careful not to call seq on it, this works fine. However, printing it at the REPL will call seq. Does the print-method impl for eduction promise too much? This is a only minor annoyance more than anything else, obviously I could create my own flavor of eduction.

Totally hypothetical example:

(defn database-index
  [name]
  (reify clojure.lang.IReduceInit
    (reduce [_ f init]
      (with-open [rdr (fressian/create-reader (io/input-stream name))]
       (loop [] ...reduce impl...)))))

(eduction (filter (as-of #inst "2012-01-01")) (database-index "eavt.fress"))
;; ^ throws when printed by repl

[1] https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L7336-L7338
[2] https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L7360

Proposed Approach:
Let eduction print like an opaque #object






[CLJ-1746] new keyword for `require` that both refers other namespace's symbol and exclude the same in clojure.core Created: 06/Jun/15  Updated: 09/Jun/15

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

Type: Enhancement Priority: Minor
Reporter: Hoang Minh Thang Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: enhancement


 Description   

I find myself repeat code like this

(ns foo.bar
(:refer-clojure :exclude [doseq])
(:require [clojure.core.typed :refer [doseq]]))

and just think why not something like:

(ns foo.bar
(:require [clojure.core.typed :override [doseq]]))



 Comments   
Comment by Mike Anderson [ 09/Jun/15 10:40 AM ]

I agree this is very annoying.

I think it is a duplicate of my issue though: The patch for CLJ-1257 would make this unnecessary (it would allow the user to override any vars, without getting an exception).





[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-1744] Clear unused locals Created: 03/Jun/15  Updated: 04/Jun/15

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

Type: Enhancement Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: compiler, locals-clearing

Attachments: Text File 0001-CLJ-1744-clear-unused-locals.patch     Text File 0001-CLJ-1744-clear-unused-locals-v2.patch    
Patch: Code

 Description   

Clojure currently doesn't clear unused locals. This is problematic as some form of destructuring can generate unused/unusable locals that the compiler cannot clear and thus can cause head retention:

;; this works
user=> (loop [xs (repeatedly 2 #(byte-array (quot (.maxMemory (Runtime/getRuntime)) 10)))] (when (seq xs) (recur (rest xs))))
nil
;; this doesn't
user=>  (loop [[x & xs] (repeatedly 200 #(byte-array (quot (.maxMemory (Runtime/getRuntime)) 10)))] (when (seq xs) (recur xs)))
OutOfMemoryError Java heap space  clojure.lang.Numbers.byte_array (Numbers.java:1252)

Here's a macroexpansion that explicits this issue:

user=> (macroexpand-all '(loop [[a & b] c] [a b]))
(let* [G__21 c 
       vec__22 G__21
       a (clojure.core/nth vec__22 0 nil)
       b (clojure.core/nthnext vec__22 1)]
 (loop* [G__21 G__21]
   (let* [vec__23 G__21
          a (clojure.core/nth vec__23 0 nil)
          b (clojure.core/nthnext vec__23 1)]
     [a b])

The first two bindings of a and b will hold onto the head of c since they are never used and not accessible from the loop body they cannot be cleared.

The attached patch allows the compiler to clear unused locals making both examples work by trivially popping the binding value off the stack rather than assigning it to the local variable if analysis reveals it's never used.

Patch: 0001-CLJ-1744-clear-unused-locals-v2.patch



 Comments   
Comment by Michael Blume [ 03/Jun/15 12:57 PM ]

Nice =)





[CLJ-1743] Avoid compile-time static initialization of classes when using inheritance Created: 02/Jun/15  Updated: 27/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Abe Fettig Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: aot, compiler, interop

Attachments: Text File 0001-Avoid-compile-time-class-initialization-when-using-g.patch     Text File clj-1743-2.patch    
Patch: Code
Approval: Triaged

 Description   

I'm working on a project using Clojure and RoboVM. We use AOT compilation to compile Clojure to JVM classes, and then use RoboVM to compile the JVM classes to native code. In our Clojure code, we call Java APIs provided by RoboVM, which wrap the native iOS APIs.

But we've found an issue with inheritance and class-level static initialization code. Many iOS APIs require inheriting from a base object and then overriding certain methods. Currently, Clojure runs a superclass's static initialization code at compile time, whether using ":gen-class" or "proxy" to create the subclass. However, RoboVM's base "ObjCObject" class [1], which most iOS-specific classes inherit from, requires the iOS runtime to initialize, and throws an error at compile time since the code isn't running on a device.

CLJ-1315 addressed a similar issue by modifying "import" to load classes without running static initialization code. I've written my own patch which extends this behavior to work in ":gen-class" and "proxy" as well. The unit tests pass, and we're using this code successfully in our iOS app.

Patch: clj-1743-2.patch

Here's some sample code that can be used to demonstrate the current behavior (Full demo project at https://github.com/figly/clojure-static-initialization):

Demo.java
package clojure_static_initialization;

public class Demo {
  static {
    System.out.println("Running static initializers!");
  }
  public Demo () {
  }
}
gen_class_demo.clj
(ns clojure-static-initialization.gen-class-demo
  (:gen-class :extends clojure_static_initialization.Demo))
proxy_demo.clj
(ns clojure-static-initialization.proxy-demo)

(defn make-proxy []
  (proxy [clojure_static_initialization.Demo] []))

[1] https://github.com/robovm/robovm/blob/master/objc/src/main/java/org/robovm/objc/ObjCObject.java



 Comments   
Comment by Alex Miller [ 18/Jun/15 3:01 PM ]

No changes from previous, just updated to apply to master as of 1.7.0-RC2.

Comment by Alex Miller [ 18/Jun/15 3:03 PM ]

If you had a sketch to test this with proxy and gen-class, that would be helpful.

Comment by Abe Fettig [ 22/Jun/15 8:31 AM ]

Sure, what form would you like for the sketch code? A small standalone project? Unit tests?

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

Just a few lines of Java (a class with static initializer that printed) and Clojure code (for gen-class and proxy extending it) here in the test description that could be used to demonstrate the problem. Should not have any dependency on iOS or other external dependencies.

Comment by Abe Fettig [ 01/Jul/15 8:49 PM ]

Sample code added, let me know if I can add anything else!

Comment by Abe Fettig [ 27/Jul/15 2:21 PM ]

Just out of curiosity, what are the odds this could make it into 1.8?

Comment by Alex Miller [ 27/Jul/15 6:06 PM ]

unknown.





[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-1741] deftype class literals and instances loaded from different classloaders when recompiling namespace Created: 30/May/15  Updated: 18/Jun/15

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

Type: Defect Priority: Major
Reporter: Stephen Nelson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: aot, classloader, compiler

Attachments: Text File 0001-CLJ-1714-Don-t-load-AOT-class-when-compiling-already.patch    
Patch: Code
Approval: Incomplete

 Description   

Scenario: Given two files:

src/dispatch/core.clj:

(ns dispatch.core (:require [dispatch.dispatch]))

src/dispatch/dispatch.clj:

(ns dispatch.dispatch)
(deftype T [])
(def t (->T))
(println "T = (class t):" (= T (class t)))

Compile first core, then dispatch:

java -cp src:target/classes:clojure.jar -Dclojure.compile.path=target/classes clojure.main
user=> (compile 'dispatch.core)
T = (class t): true
dispatch.core
user=> (compile 'dispatch.dispatch)
T = (class t): false     ;; expected true
dispatch.dispatch

This scenario more commonly occurs in a leiningen project with :aot :all. Files are compiled in alphabetical order with :all. In this case, dispatch.core will be compiled first, then dispatch.dispatch.

Cause:

(compile 'dispatch.core)

  • transitively compiles dispatch.dispatch
  • writes .class files to compile-path (which is on the classpath)
  • assertion passes

(compile 'dispatch.dispatch)

  • due to prior compile, load dispatch.dispatch__init is loaded via the appclassloader
  • ->T constructor will use new bytecode to instantiate a T instance - this uses appclassloader, loaded from compiled T on disk
  • however, T class literals are resolved with RT.classForName, which checks the dynamic classloader cache, so uses old runtime version of T, instead of on-disk version

In 1.6, RT.classForName() did not check dynamic classloader cache, so loaded T from disk as with instances. This was changed in CLJ-979 to support other redefinition and AOT mixing usages.

Approaches:

1) Compile in reverse dependency order to avoid compiling twice.

Either swap the order of compilation in the first example or specify the order in project.clj:

:aot [dispatch.dispatch dispatch.core]

This is a short-term workaround.

2) Move the deftype into a separate namespace from where it is used so it is not redefined on the second compile. This is another short-term workaround.

3) Do not put compile-path on the classpath (this violates current expectations, but avoids loading dispatch__init)

(set! *compile-path* "foo")
(compile 'dispatch.core)
(compile 'dispatch.dispatch)

This is not easy to set up via Leiningen currently.

4) Compile each file with an independent Clojure runtime - avoids using cached classes in DCL for class literals.

Probably too annoying to actually do right now in Leiningen or otherwise.

5) Make compilation non-transitive. This is in the ballpark of CLJ-322, which is another can of worms. Also possibly where we should be headed though.

Screening: I do not believe the proposed patch is a good idea - it papers over the symptom without addressing the root cause. I think we need to re-evaluate how compilation works with regard to compile-path (#3) and transitivity (CLJ-322) (#5), but I think we should do this after 1.7. - Alex

See also: CLJ-1650



 Comments   
Comment by Alex Miller [ 30/May/15 8:50 PM ]

Pulling into 1.7 for consideration.

Comment by Stephen Nelson [ 30/May/15 8:55 PM ]

I've added a debug flag to my example that causes type instance hashcodes and their class-loaders to be printed.

Compiling dispatch.core
deftype => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
defmethod => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
instance => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
dispatch:  :pass
Compiling dispatch.dispatch
deftype => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
defmethod => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
instance => 760357227 (sun.misc.Launcher$AppClassLoader@42a57993)
dispatch:  :fail
Comment by Nicola Mometto [ 01/Jun/15 7:23 AM ]

The compiler has weird loading rules when using `compile` and both a clj file and a class file are present in the classpath.

This bug happens because RT.load will load the AOT class file rebinding the ->Ctor to use the AOT deftype instance.

A fix for this would be making load "loaded libs" aware to avoid unnecessary/harmful reloadings.

Comment by Nicola Mometto [ 01/Jun/15 10:55 AM ]

The attached patch fixes this bug by keeping track of what has already been loaded and loading the AOT class only if necessary

Comment by Alex Miller [ 16/Jun/15 2:24 PM ]

Original description (since replaced):

Type-dispatching multimethods are defined using the wrong type instance

When using a multimethod that dispatches on types, such as print-dup/print-method, the type reference passed to addMethod in the presence of aot is incorrect on the second load of the namespace. This means that if the namespace has already been loaded as a dependency of another file, the second load when the namespace is loaded for aot compilation will produce a multimethod that fails to dispatch correctly.

I've created an example repository:
https://github.com/sfnelson/clj-mm-dispatch

To reproduce independently, create a namespace that contains a deftype and a multimethod dispatching on the type, and a second namespace that requires the first and sorts alphabetically before the first. Aot-compile both namespaces. When the type-defining namespace is loaded via require it produces a class file for the deftype. When it is loaded the second time for aot-compilation, the type corresponding to the existing class file is given to the defmethod, instead of the new class constructed by loading the namespace. This causes the multimethod it fail to dispatch correctly.

To me this issue seems similar to CLJ-979: the type passed to the multimethod is retrieved using the wrong classloader. This suggests that it might have wider implications than AOT and multimethod dispatch.

Comment by Nicola Mometto [ 18/Jun/15 11:09 AM ]

I just realized this ticket is a duplicate of CLJ-1650





[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-1737] Omit java exception class from CompilerException message Created: 23/May/15  Updated: 23/May/15

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

Type: Enhancement Priority: Minor
Reporter: John Hume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, errormsgs, patch, usability

Attachments: Text File clearer-CompilerException-messase.patch     File compiler_exception_examples.clj    
Patch: Code

 Description   

A CompilerException is always created with a cause exception. Currently the message is built using cause.toString(), which for all examples I've examined is the cause class, followed by a colon, followed by the cause message. In all those examples, the message of the cause is informative, and the class name provides no additional help.

I propose to switch to using cause.getMessage() rather than cause.toString(). This would make it easier for tools to present compiler errors that don't leak implementation details that may confuse a new user. The cause class would still be shown in the stack trace.

Here are the examples I looked at, with the output from before the attached patch:

Example source '(ns foo)

(def'
Exception message:
 java.lang.RuntimeException: EOF while reading, starting at line 3, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:3:1)

Example source ':foo}'
Exception message:
 java.lang.RuntimeException: Unmatched delimiter: }, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:6)

Example source 'foo'
Exception message:
 java.lang.RuntimeException: Unable to resolve symbol: foo in this context, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:14:1)

Example source 'clojure.core/firstt'
Exception message:
 java.lang.RuntimeException: No such var: clojure.core/firstt, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:15:1)

Example source '(nil 1)'
Exception message:
 java.lang.IllegalArgumentException: Can't call nil, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '("hi" 1)'
Exception message:
 java.lang.ClassCastException: java.lang.String cannot be cast to clojure.lang.IFn, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '{:foo}'
Exception message:
 java.lang.RuntimeException: Map literal must contain an even number of forms, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:7)

Example source '1st'
Exception message:
 java.lang.NumberFormatException: Invalid number: 1st, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

And the output with the attached patch applied:

Example source '(ns foo)

(def'
Exception message:
 EOF while reading, starting at line 3, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:3:1)

Example source ':foo}'
Exception message:
 Unmatched delimiter: }, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:6)

Example source 'foo'
Exception message:
 Unable to resolve symbol: foo in this context, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:14:1)

Example source 'clojure.core/firstt'
Exception message:
 No such var: clojure.core/firstt, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:15:1)

Example source '(nil 1)'
Exception message:
 Can't call nil, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '("hi" 1)'
Exception message:
 java.lang.String cannot be cast to clojure.lang.IFn, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '{:foo}'
Exception message:
 Map literal must contain an even number of forms, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:7)

Example source '1st'
Exception message:
 Invalid number: 1st, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)





[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-1734] Display more descriptive error message when trying to use reader conditionals in a non-cljc file Created: 19/May/15  Updated: 20/May/15

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

Type: Enhancement Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, reader


 Description   

I spent a few puzzled minutes trying to understand the following message from the Clojure compiler:

CompilerException java.lang.RuntimeException: Conditional read not allowed, compiling: <filename>

Eventually I realised it was because I was trying to use reader conditionals in a .clj file that I hadn't renamed to cljc. I think it would be really helpful for people working in mixed clj and cljc codebases to have this error message extended to something like:

"Conditional read not allowed because file does not have extension .cljc"



 Comments   
Comment by Alex Miller [ 19/May/15 11:45 PM ]

The reader doesn't know this - it can be called in multiple ways (from repl, via clojure.core/read, via clojure.core/read-string, load/compile .cljc, load/compile .clj) so that description would actually be wrong in some of those. It seems like you're getting a pretty good error message already - it told you the problem and gave you the file name.

The message could be tweaked to something like "Reader conditionals not allowed in this context" which might give you a better clue.

Comment by Daniel Compton [ 20/May/15 3:12 PM ]

Perhaps I'm not understanding how the reader determines whether reader conditionals are allowed or not, but those would all seem to have different reasons for not being allowed and would be caught by different checks. Each of these checks could give a more specific warning explaining why the read wasn't allowed?

Counteracting my point, it looks like there is only one place where this exception is thrown - https://github.com/clojure/clojure/blob/7b9c61d83304ff9d5f9feddecf23e620c0b33c6e/src/jvm/clojure/lang/LispReader.java#L1406. I'm not sure if this could be extended to give more details in different error cases or if that information isn't available at that point?

Comment by Alex Miller [ 20/May/15 3:36 PM ]

The reader is invoked with an options map which will (or will not) have {:read-cond :allow} or {:read-cond :preserve}. That's the only info the reader has - if either of those is set and a reader conditional is encountered, it throws.

The compiler decides how to initialize these options when it's calling the reader. Users of read and read-string similarly decide which options are allowed when they call it. It would be possible to pass more info into the reader or to catch and rethrow in the compiler where more context is known, but both of those complicate the code for what is already a decent error imho.

Comment by Daniel Compton [ 20/May/15 4:03 PM ]

I agree it is a reasonable error message, I guess we can wait and see how other people find it once 1.7 is released. If it turns out to be an issue for lots of other people then we could revisit this then?





[CLJ-1733] print-dup form unreadable for sorted sets and maps Created: 19/May/15  Updated: 31/Jul/15

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

Type: Defect Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Clojure 1.6.0
Clojure 1.7.0-alpha5
Clojure 1.7.0-beta3

java version "1.8.0"
Java(TM) SE Runtime Environment (build 1.8.0-b132)
Java HotSpot(TM) 64-Bit Server VM (build 25.0-b70, mixed mode)


Attachments: Text File clj-1733-tagged-literals-throw-on-sorted-set.patch    
Patch: Code and Test
Approval: Triaged

 Description   

print-dup for sorted sets and maps presume a nonexistent static create method that takes an IPersistentCollection

Printing

user=> (print-dup (sorted-set 1) *out*)
#=(clojure.lang.PersistentTreeSet/create [1])

Can't read back

(read-string "#=(clojure.lang.PersistentTreeSet/create [1])")
ClassCastException Cannot cast clojure.lang.PersistentVector to clojure.lang.ISeq  java.lang.Class.cast (Class.java:3356)

Possible Fixes

  • add create methods taking IPersistentVector to collections
  • emit something different from print-dup


 Comments   
Comment by Alex Miller [ 19/May/15 4:55 PM ]

It's trying to invoke PersistentTreeSet.create(ISeq) with ["123"]. It's not clear to me where the vector comes from?

Comment by Nikita Prokopov [ 19/May/15 5:04 PM ]

It’s a particular case of CLJ-1461. Vector comes from reading output of print-dup:

(defrecord Rec [f])

(binding [*print-dup* true]
  (prn (Rec. (sorted-set 1))))
;; => #tonsky.Rec[#=(clojure.lang.PersistentTreeSet/create [1])]

I already have a patch for PersistentTreeSet (attached here). Can look into CLJ-1461 later.





[CLJ-1732] Add docstring explanation of (isa? [x1 x2...] [parent1 parent2...]) Created: 17/May/15  Updated: 17/May/15

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Approval: Triaged

 Description   

The "Multimethods and Hierarchies" page mentions that "isa?" has special behavior when aimed at two vectors[1]. But the docstring of "isa?" does not mention it[2].

[1] http://clojure.org/multimethods
[2] http://clojure.github.io/clojure/clojure.core-api.html#clojure.core/isa?






[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-1730] Improve `refer` performance Created: 13/May/15  Updated: 13/May/15

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

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

Attachments: Text File refer-perf.patch    
Patch: Code
Approval: Triaged

 Description   

refer underlies require, use, and refer-clojure use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

Approach: Some improvements that can be made:

  • Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
  • Use transients to build map of references to refer
  • Instead of cas'ing each new reference individually, build map of all changes, then cas
  • For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements in practice are modest.

Performance:

expr in a new repl 1.7.0-beta3 1.7.0-beta3+patch
(in-ns 'foo) (clojure.core/refer 'clojure.core) 2.65 ms 0.994 ms
(in-ns 'bar) (clojure.core/refer 'clojure.core :only '[inc dec]) 1.04 ms 0.113 ms
(use 'criterium.core) 0.877 ms 0.762 ms
(require '[clojure.core.async :refer (>!! <!! chan close!)]) 3408 ms 3302 ms

Patch: refer-perf.patch






[CLJ-1729] Make Counted and count() return long instead of integer Created: 12/May/15  Updated: 08/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

Currently count() returns an int - should bump that up to a long.

On long overflow, count() should throw ArithmeticException. Also see CLJ-1229.



 Comments   
Comment by Erik Assum [ 07/Jul/15 9:24 AM ]

Looking at this, there are some problems like in
clojure.lang.RT#toArray line 1658
where you create a new Object array based on the count of a collection.
It seems as if new Object[] takes an int as a param, so one would have to downcast the long to an int for this to work.

Comment by Alex Miller [ 07/Jul/15 9:39 AM ]

If you're creating an Object[] greater than 2147483647, you may have other problems.

But yes, this ticket definitely needs a more thorough analysis as to what is affected. In this case, I think if the count is <= Integer/MAX_VALUE, then it should proceed and otherwise should throw an exception.

Comment by Erik Assum [ 08/Jul/15 8:19 AM ]

hmmm, this also causes problems wrt java.util.Collection size:
clojure.lang.APersistentSet#size line 164
Where size is specified by

http://docs.oracle.com/javase/7/docs/api/java/util/Collection.html#size()





[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: 04/May/15

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

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

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

 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-1721] Enable test case for char? Created: 03/May/15  Updated: 03/May/15

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: patch

Attachments: Text File CLJ-1721_v01.patch    
Patch: Code and Test

 Description   

clojure.core/char? already exists, but there was no test for it (despite a comment suggesting one).






[CLJ-1720] Add clojure.core/pattern? predicate Created: 03/May/15  Updated: 03/May/15

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: patch

Attachments: Text File CLJ-1720_v01.patch     Text File CLJ-1720_v02.patch    
Patch: Code and Test

 Description   

Just like http://dev.clojure.org/jira/browse/CLJ-1719 , this helps with clj/cljs compatibility.



 Comments   
Comment by Brandon Bloom [ 03/May/15 2:37 PM ]

See also http://dev.clojure.org/jira/browse/CLJS-1242

Comment by Brandon Bloom [ 03/May/15 2:48 PM ]

Whoops, uploaded wrong patch. Tests actually pass in this v02 patch.





[CLJ-1719] Add clojure.core/boolean? predicate Created: 03/May/15  Updated: 03/May/15

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: patch

Attachments: Text File CLJ-1719_v01.patch    
Patch: Code and Test

 Description   

Having this predicate aids with clj/cljs compatibility.



 Comments   
Comment by Brandon Bloom [ 03/May/15 2:32 PM ]

See also: http://dev.clojure.org/jira/browse/CLJS-1241





[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: 05/Jun/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.





[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-1715] Use AFn.applyToHelper rather than IFn.applyTo in InvokeExpr.eval Created: 23/Apr/15  Updated: 23/Apr/15

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

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

Attachments: Text File 0001-CLJ-1715-Use-AFn.applyToHelper-rather-than-IFn.apply.patch    
Patch: Code

 Description   

Because of implementation details of how def forms are compiled, invokations in its args are evaluated using applyTo rather than directly using the invoke method. This forces IFn implementors to implement applyTo even when not necessary.

The proposed patch changes InvokeExpr.eval to use AFn.applyToHelper, so that invoke rather than applyTo is used when possible.

Example of currently failing code that will work with patch:

user=> (deftype x [] clojure.lang.IFn (invoke [_] 1))
user.x
user=> (def a ((x.)))
AbstractMethodError   clojure.lang.Compiler$InvokeExpr.eval (Compiler.java:3553)





[CLJ-1714] Some static initialisers still run at compile time if used in type hints Created: 22/Apr/15  Updated: 12/Aug/15

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

Type: Defect Priority: Minor
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler, typehints

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

 Description   

AOT compiling on an x86 machine to be run on an ARM machine when a Java dependency has a native component and the class with the native dependency is used in a type hint.

In this situation, the only native library available on the classpath is the ARM dependency, and obviously won't load on the compiling x86 machine. Java libraries tend to load the native dependencies in the static initialiser of the class, which will fail in this situation as the architecture is x86 and the dependencies are ARM, for which reason CLJ-1315 made the change to not run static initialisers at compile time.

This covers a case which didn't come up as part of CLJ-1315, that the same problem occurs if rather than constructing the class, you simply use it as a type hint (which IMO is doubly surprising as something to have a side-effect).

This patch fixes that - happy to try and create a test, but would appreciate some advice on the shape such a test would take - presumably loading a java native library would be undesirable. I could simply check for static initialisers being run, but first would need some agreement that this is universally undesirable at compile time.

I have been using this patch in production for over a year with no adverse effects (as has anybody using the clojure-android build of clojure).



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

I think this might have been logged already but I'm not sure.

Comment by Michael Blume [ 22/Apr/15 12:30 PM ]

Patch won't apply to master for me

Comment by Adam Clements [ 22/Apr/15 2:39 PM ]

Really sorry, don't know what happened there. I checked out a fresh copy of the repo and re-applied the changes, deleted the old patch as it was garbage. Try the new one, timestamped 2:37pm

Comment by Stuart Halloway [ 30/Jul/15 1:52 PM ]

Please add an example of the problem, and if possible a failing test.

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

Reset to "Open" as moving from Triaged->Incomplete is not valid in our current workflow.

Comment by Adam Clements [ 31/Jul/15 10:56 AM ]

Example problem:
AOT compiling on an x86 machine to be run on an ARM machine when a Java dependency has a native component and the class with the native dependency is used in a type hint.

In this situation, the only native library available on the classpath is the ARM dependency, and obviously won't load on the compiling x86 machine. Java libraries tend to load the native dependencies in the static initialiser of the class, which will fail in this situation as the architecture is x86 and the dependencies are ARM, for which reason CLJ-1315 made the change to not run static initialisers at compile time.

This covers a case which didn't come up as part of CLJ-1315, that the same problem occurs if rather than constructing the class, you simply use it as a type hint (which IMO is doubly surprising as something to have a side-effect).

This patch fixes that - happy to try and create a test, but would appreciate some advice on the shape such a test would take - presumably loading a java native library would be undesirable. I could simply check for static initialisers being run, but first would need some agreement that this is universally undesirable at compile time.

I have been using this patch in production for over a year with no adverse effects (as has anybody using the clojure-android build of clojure).

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

Hi Adam,

Thanks for the quick response. I think checking for static initializers being run is OK for a test.

Comment by Adam Clements [ 12/Aug/15 9:12 AM ]

Added failing tests which now pass





[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-1708] Volatile mutable in deftype is not settable when using try..finally and returning this Created: 17/Apr/15  Updated: 31/Jul/15

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

Type: Defect Priority: Minor
Reporter: Patrick Gombert Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, deftype
Environment:

clojure 1.6.0, clojure 1.7.0-beta1


Approval: Triaged

 Description   

Reproducible Code: https://gist.github.com/patrickgombert/1bcb8a051aeb3e82d855

When using a volatile-mutable field in deftype, compilation fails if the field is set! in a method call that uses both try..finally and returns itself from the method call. Leaving out either the try..finally or returning itself from the method causes compilation to succeed.

Expected behavior: set! should set the volatile-mutable variable and compilation should succeed.



 Comments   
Comment by Kevin Downey [ 17/Apr/15 7:15 PM ]

this must be the same issue as CLJ-1422 and CLJ-701, it has nothing to do with returning `this`, but with the try being in a tail position or not. if the try is not in a tail position the compiler hoists it out in to a thunk. effectively the code is

(deftype SomeType [^:volatile-mutable foo]
  SomeProtocol
  (someFn [_] ((fn [] (try (set! foo 1))))))

which the compiler also rejects, because it doesn't let you mutate fields from functions that are not the immediate protocol functions





[CLJ-1707] conditional form is not consumed when :read-allow is falsey Created: 15/Apr/15  Updated: 15/Apr/15

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

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


 Description   
user=> (def a (java.io.PushbackReader. (java.io.StringReader. "#?(:clj [1 2])")))
#'user/a
user=> (read a)
RuntimeException Conditional read not allowed  clojure.lang.Util.runtimeException (Util.java:221)
user=> (read a)
(:clj [1 2])

the expected result would be an EOF exception on the second read.






[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-1705] vector-of throws NullPointerException if given unrecognized type Created: 14/Apr/15  Updated: 06/May/15

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

Type: Defect Priority: Minor
Reporter: John Croisant Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: checkargs, errormsgs, newbie
Environment:

MacOS X Version 10.9.5

java version "1.8.0_11"
Java(TM) SE Runtime Environment (build 1.8.0_11-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.11-b03, mixed mode)


Attachments: Text File clj-1705-2.patch     Text File clj-1705-3.patch     Text File CLJ-1705.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Summary:

If the user passes an unrecognized type keyword to vector-of, it will throw a NullPointerException with no message. This gives no indication to the user of what the problem is, which can frustrate the user and make debugging harder than it needs to be.

Example:

user=> (vector-of :integer 1 2 3) ; user meant (vector-of :int 1 2 3)

NullPointerException   clojure.core/vector-of (gvec.clj:472)

Approach: Throw more informative error message than NPE with more info.

After:

user=> (vector-of :integer 1 2 3)
IllegalArgumentException Unrecognized type :integer  clojure.core/vector-of (gvec.clj:498)

Patch: clj-1705-3.patch



 Comments   
Comment by Johan Mena [ 05/May/15 12:12 AM ]

Here’s my proposed solution: https://github.com/jhn/clojure/commit/0522fffd7893e8c79969f5c6ac91a333484c8e23 — It works as expected and the tests pass.

One more thing I'm not sure about is how to create a patch for this. The ticket mentions "Release 1.4, Release 1.6” as affected versions, but I found this to still be present in 1.7 and in 1.5 too. Do I checkout only these two tags (1.4, 1.6) from the git repo and create a patch for each one separately? Or what's the usual way to go about it? I read through the Issue Tracking section of the wiki but it's still not very clear.

Thanks.

Johan

Comment by Alex Miller [ 05/May/15 7:09 AM ]

in general, just work from master. We don't generally back port to older versions.

You should make a patch following the instructions at http://dev.clojure.org/display/community/Developing+Patches

Comment by Andy Fingerhut [ 05/May/15 10:20 AM ]

Johan, are you listed as "johan (jhn)" on the contributor list here? http://clojure.org/contributing

If so, it would be good to update that listing to something like "Johan Mena (jhn)" for the time when someone needs to check that you have signed a Clojure CA, before your patch is committed.

Comment by Alex Miller [ 05/May/15 10:35 AM ]

I believe that's correct - I have updated the contributing page.

Comment by Johan Mena [ 05/May/15 1:57 PM ]

The patch I attached yesterday did start from master, so I think it's good to go for review.

Thanks Alex & Andy!

Comment by Alex Miller [ 06/May/15 9:56 AM ]

Ok, I looked at the patch. Functionally, seems ok other than that the find+destructuing doesn't seem to have a use over get.

However, I also looked at performance for the happy path using criterium and the following test:

(quick-bench (vector-of :int 1 2))

Before the patch: 27.418350 ns
After the patch: 79.883695 ns

The reason for this is that the prior code created a single map and constructed the array managers in there once, whereas the patch causes the array manager to be created each time through the code.

Re-extracting the map and replacing the find with get, I was back to: 34.320916 ns

I think that's too much of a hit for this change so I looked at a couple variants:

  • def'ing each am separately and using a `case` expression - 36.566750 ns
  • using `or` instead of if-let - 29.680252 ns

I think that last one is pretty close, but we're paying a hit just to invoke the new function, so my final stab was turning that into a macro - 28.411626 ns, which seems tolerable to me. Because I used a macro, I also had to move the type hints in vector-of to avoid reflection.

Since I had everything sitting here, I went ahead and made a new patch. I feel bad about replacing yours, but not sure what else makes sense to do.

Comment by Alex Miller [ 06/May/15 10:02 AM ]

Ok, I split up the patch into test and code commits so you have credit for some of the changes! Since it's got my stuff in here, this will have to wait till it gets added to 1.8 to get screened by someone else.

Comment by Johan Mena [ 06/May/15 11:04 AM ]

Haha, you shouldn't have bothered, but thanks!

And thanks for the detailed explanation! Actually 'get' was my first approach too but the (get map key not-found) form kept throwing the exception in the `not-found` part even in the happy case. I asked around and found out the else part needs to be evaluated in this case too. Someone suggested using a macro but I'm not very familiar with that just yet, so I went with if-let, which seemed readable. Just out of curiosity, when you tried the get approach, did you use (get map key) and check for nil to throw the exception?

I ran my code through the irc channel and got positive feedback, so completely forgot about performance. Will keep it in mind next time.





[CLJ-1704] Clarify cond documentation to explain about using :else Created: 14/Apr/15  Updated: 14/Apr/15

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

Type: Enhancement Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring


 Description   

The documentation for cond doesn't explicitly mention that you can use :else (or any other keyword) to catch any values that don't match the previous conditions. While it is true that the documentation does say that a test will evaluate and return the value of logical true, it could be more helpful by pointing out that a keyword like :else will always be logical true.

I'm not 100% sure about whether this is necessary, but wanted to see what others thought and whether it would be helpful or not.






[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-1702] Silent fail on unspecified map destructuring Created: 13/Apr/15  Updated: 16/Apr/15

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

Type: Enhancement Priority: Trivial
Reporter: Linus Ericsson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: destructuring


 Description   

When accidentally switching keyword and (previously undefined) symbol in map destructuring, an error is correctly thrown:

(let [{:b b} {:b 1}] b)

=> CompilerException java.lang.RuntimeException: Unable to resolve symbol: b in this context, compiling: (/tmp/form-init7939480206147277345.clj:1:1)

When the symbol ("a" used below) is defined, however, there is a more subtle error:

(def a 0)
(let [{:a a} {:a 1}] a)
=> nil

Expected: Destructuring should only accept the defined keywords :or, :keys, :as, :strs and :syms as keys in a destructuring map.



 Comments   
Comment by Michael Blume [ 14/Apr/15 1:35 PM ]

This may be a duplicate of CLJ-1613

Comment by Linus Ericsson [ 16/Apr/15 9:13 AM ]

Michael, I do think this is a somewhat different problem.





[CLJ-1701] Serialization of protocol methods broken: java.io.NotSerializableException: clojure.lang.MethodImplCache Created: 13/Apr/15  Updated: 20/Apr/15

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

Type: Defect Priority: Major
Reporter: Dr. Christian Betz Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

With this test, you can see that we cannot serialize methods from protocols (i.e. time-from-tweet), as this results in a java.io.NotSerializableException: clojure.lang.MethodImplCache
at java.io.ObjectOutputStream.writeObject0 (ObjectOutputStream.java:1183)
java.io.ObjectOutputStream.defaultWriteFields (ObjectOutputStream.java:1547)
java.io.ObjectOutputStream.writeSerialData (ObjectOutputStream.java:1508)
java.io.ObjectOutputStream.writeOrdinaryObject (ObjectOutputStream.java:1431)
java.io.ObjectOutputStream.writeObject0 (ObjectOutputStream.java:1177)
java.io.ObjectOutputStream.writeObject (ObjectOutputStream.java:347)
sparkling.protocol_test$serialize.invoke (protocol_test.clj:11)

This is the actual test:

(ns sparkling.protocol-test
(:require [clojure.test :refer :all])
(:import [java.io ObjectInputStream ByteArrayInputStream ObjectOutputStream ByteArrayOutputStream]))

(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)))

(defn- deserialize
"Deserializes and returns a single object from the given byte array."
[bytes]
(with-open [ois (-> bytes ByteArrayInputStream. ObjectInputStream.)]
(.readObject ois)))

(defprotocol timestamped
(time-from-tweet [item]))

(defrecord tweet [username tweet timestamp]
timestamped
(time-from-tweet [_]
timestamp
))

(deftest sequable-serialization
(testing "Serialization of function"
(let [item identity]
(is item (-> item serialize deserialize))))

(testing "Serialization of protocol method"
(let [item time-from-tweet]
(is item (-> item serialize deserialize)))))



 Comments   
Comment by Dr. Christian Betz [ 13/Apr/15 6:15 AM ]

BTW: Same is true for multimethods, here the exception is java.io.NotSerializableException: clojure.lang.MultiFn

Comment by Alex Miller [ 13/Apr/15 9:35 AM ]

I don't think we expect functions to be serializable in this way. Both protocols and multimethods effectively have runtime state based on what implementations have extended them. What would it mean to serialize these functions? Would you serialize them with whatever implementations have been loaded at that point? Or with none? Both seem problematic to me. Regular functions are closures and can capture the state of their environment. I think better answers are either AOT or for regular functions, something like the serializable-fn library.

Comment by Dr. Christian Betz [ 13/Apr/15 1:13 PM ]

Hi,

thanks for the comments. First, something to the background: I'm developing Sparkling, a Clojure API to Apache Spark. For distributing code in the cluster it depends on AOT compiled functions, so yes, you cannot simply serialize any function around, it needs to be AOT'd. Serializiation provides us with support for the current bindings etc, and everything works as expected. So, AFunction is serializable for a reason and so are other implementations of AFn/IFn, everything works well.

Regarding the state of protocols and multimethods - I think it's conceptually the same as the state of functions (which function definition, the var might be bound multiple times, etc.), and the closures given in bindings. There's no reason for me as the user of a protocol to believe that the method from the protocol differs from a function. In fact (ifn? protocol-method) also returns true.

serializable-fn, not being intended for over-the-wire serialization in the first place, has problems with collections of functions in bindings of the serializble function, together with an issue with PermGen pollution by creating classes for the same function over and over again in the context of Spark.

I think I'm fine for the moment, as I can wrap the protocol method in a function, but I still believe, that this is a bug.

Regards

Chris

Comment by Dr. Christian Betz [ 20/Apr/15 3:10 AM ]

actually, this is the code snippet from clojure.lang.AFunction causing the pain:

clojure.lang.AFunction.java
public abstract class AFunction extends AFn implements IObj, Comparator, Fn, Serializable {

public volatile MethodImplCache __methodImplCache;

AFunction is serializable, but MethodImplCache is not. I'm not sure if it's enough to mark it as transient, because I did not check where initialization happens.

Comment by Dr. Christian Betz [ 20/Apr/15 3:29 AM ]

My comment per mail got lost in SMTP-nirvana: There's an easy workaround. Wrap the protocol method in a function, that will do the trick at the cost of uglifying your code





[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.mai