<< Back to previous view

[CLJ-2434] Improve no such field exception message Created: 16/Nov/18  Updated: 16/Nov/18

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

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

Attachments: Text File clj-2434.patch    
Patch: Code
Approval: Prescreened

 Description   

Not a particularly friendly error message:

=> (String/foo)
Syntax error compiling . at (REPL:1:1).
foo

The first line looks like some kind of bad message but this expands to (. String foo) and the expression being compiled here is literally the special form ".". The second line is the message from NoSuchFieldException (thanks Java, very helpful).

We're already catching and rethrowing that though, and could make it whatever we want (don't think the cause stack or exception are any use).

Proposed:

user=> (String/foo)
Syntax error compiling . at (REPL:1:1).
No field foo on class java.lang.String

Patch: clj-2434.patch






[CLJ-2433] Make set functions in clojure.set coerce its arguments to set Created: 16/Nov/18  Updated: 16/Nov/18

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

Type: Enhancement Priority: Major
Reporter: Erik Assum Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Approval: Triaged

 Description   

There are a number of tickets concerned with the fact that the set functions in clojure.set misbehave when passed arguments that are not sets.

This set of issues include CLJ-810, CLJ-1087, CLJ-1682, and CLJ-1954

The functions affected by this are:

  1. difference
  2. intersection
  3. union
  4. subset?
  5. superset?

as these are known to produce unexpected results when passed non-set arguments.

Problem
As the above mentioned issues suggest, todays implementation of these functions leads to confusion and erroneous results when called with non-set input. The user is given no warning or indications of the error he's making.

Possible solutions

  1. Add a coercion to set on the arguments said functions
  2. Throw an exception when the arguments are not sets
  3. Handle this with clojure.spec
  4. Leave it as is

Tradeoffs

  1. Given CLJ-2362, which makes a call to set close to a noop, the coercion should not incur much of a performance penalty. It has been argued that the code might even be faster, as type hints can be given and the compiler/jit might make better choices. For the common mistakes (passing vectors/lists instead of sets) it should be backwards compatible
  2. Throwing an exception on non-set arguments would break programs which work correctly today (although by chance), such as data.diff.
  3. Handling it with clojure.spec seems like a viable option, but again, this would break data.diff if the functions were spec'ed to both receive and return sets.
  4. Leaving it as it is, and we will continue to surprise both new and old clojurists.

Evidence of this being a problem

  1. The tickets mentioned above seem to indicate that people stumble upon this often enough to file issues
  2. https://clojuredocs.org/clojure.set/superset_q#example-5b5acd38e4b00ac801ed9e39


 Comments   
Comment by Alex Miller [ 16/Nov/18 1:34 PM ]

Before being considered, this ticket needs:

  • a good problem statement
  • an assessment of where existing code calls these functions with something other than sets
  • a table of alternatives and their tradeoffs. presumably alternatives are: add specs, add validation checks, add coercions, etc. Tradeoffs may include effects on existing callers (known or unknown), performance, etc.

Decisions to make:

  • Are existing calls (with inputs that are not sets) valid or invalid?
  • What change should be made in the functions

Patches are probably not super useful until those things are decided.





[CLJ-2432] datafy should lazy load reflect Created: 16/Nov/18  Updated: 16/Nov/18  Resolved: 16/Nov/18

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

Type: Defect Priority: Major
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-2432-3.patch     Text File clj-2432-4.patch     Text File clj-2432-5.patch     Text File lazy-datafy-reflect-2.patch     Text File lazy-datafy-reflect.patch    
Patch: Code
Approval: Ok

 Description   

Patch: clj-2434-5.patch



 Comments   
Comment by Alex Miller [ 16/Nov/18 12:46 PM ]

-4 patch removes unnecessary call to symbol (and squashes), retains attribution

Comment by Stuart Halloway [ 16/Nov/18 1:14 PM ]

-5 patch clarifies docstring and guards against illegal arguments





[CLJ-2430] Elevate phase in throwable data and conveyance for prepl Created: 15/Nov/18  Updated: 16/Nov/18  Resolved: 16/Nov/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: error-reporting

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

 Description   

Problems:

  • phase is currently buried in ex-data of top wrapper exception
  • prepl is conveying exception phase in message instead of in exception data
  • error phases are not currently documented
  • LispReader$ReaderException is not an exception info

Changes:

  • clojure.core.server:
    • prepl sets error phase in throwable data
    • messages for thrown exception are marked with `:exception` flag
  • clojure.core-print:
    • Improve docstring for StackTraceElement->vec
    • Improve docstring for Throwable->map
    • Elevate phase to top-level :phase key in Throwable->map
  • clojure.main
    • Fix reflection warning in core-class?
    • Add phase list in ex-triage docstring
    • Updated ex-triage to handle changes in :read-source and :read-eval-result phases
    • Simplified code for exceptions thrown in clojure.main/repl
  • LispReader
    • Make ReaderException an IExceptionInfo

Patch: clj-2430-2.patch



 Comments   
Comment by Alex Miller [ 16/Nov/18 9:25 AM ]

-2 fixes a bug that lose the phase in ex-triage





[CLJ-2431] clojure.java.io/resource NPEs when the context ClassLoader is null Created: 15/Nov/18  Updated: 15/Nov/18

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

Type: Defect Priority: Major
Reporter: mccraigmccraig Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: java.io
Environment:

clojure 1.9


Approval: Triaged

 Description   

clojure.java.io/resource assumes the current Thread's context ClassLoader will be non-null. if the Thread's context ClassLoader is null then it NPEs

(resource n (.getContextClassLoader (Thread/currentThread)))

the javadocs indicate it may reasonably be null, and i'm seeing a case in the wild where it's non-null - https://docs.oracle.com/javase/8/docs/api/java/lang/Thread.html#getContextClassLoader--

clojure.java.io/resource should perhaps default to using (ClassLoader/getSystemClassLoader) when the context ClassLoader is null






[CLJ-2429] datafy JavaReflector Created: 14/Nov/18  Updated: 15/Nov/18  Resolved: 15/Nov/18

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: datafy, java.reflect

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

 Description   

Approach: Use metadata extension extend the symbols returned by typesym so that they datafy to the associate class.

Implementation notes:

  • Not dealing with AsmReflector, this could be done later as a compatible enhancement.
  • Also needed to fix a bug in do-reflect, which could not handle classes whose typename does not roundtrip through Class/forName, e.g. arrays in primitives.





[CLJ-2333] Support java.nio.file.Path in clojure.java.io Created: 07/Mar/18  Updated: 14/Nov/18

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

Type: Enhancement Priority: Major
Reporter: Michael Nygard Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: io

Attachments: Text File CLJ-2333-1.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

java.nio.file.Path objects are largely equivalent to java.io.File. They represent a location in a (possibly-remote) filesystem.

Coercions in clojure.java.io don't recognize java.nio.file.Path. It would be nice if they did.

Prescreened: Alex Miller

Patch: CLJ-2333-1.patch



 Comments   
Comment by Andrew Oberstar [ 27/Jun/18 9:37 PM ]

Added a patch that supports java.nio.file.Path in Coercions and IOFactory. There are more places (particularly the do-copy multimethod) that might warrant Path support, but this seemed like a good start.

Note, in case there's any confusion, I have signed the CA (June 17th), though my name is not yet on the site's list of CA signers.

Comment by Arne Brasseur [ 14/Nov/18 2:19 AM ]

java.nio.file.Path was added in Java 7, which is perhaps why this change didn't make it in before. Clojure 1.10 will require Java 8, so perhaps it's time to consider this patch.





[CLJ-2428] Separate compiler exceptions into :compilation and :compile-syntax-check Created: 13/Nov/18  Updated: 13/Nov/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, error-reporting


 Description   

With the new error reporting phases in Clojure 1.10, we identified two separate phases - :compile-syntax-check (for syntax errors found by compiler) and :compilation for errors that occur during compilation but that are not syntax errors.

Example of the latter:

user=> a/b
Syntax error compiling at (REPL:0:0).
No such namespace: a
user=> (ex-data *e)
#:clojure.error{:phase :compile-syntax-check, :line 0, :column 0, :source "NO_SOURCE_PATH"}

The syntax here is fine, just a is not a known namespace.

The Compiler adheres to no patterns in differentiating between these two. We should go through and make this consistent, either through exception types or explicitly specifying the phase in a CompilerException.






[CLJ-2427] Exceptions thrown in user namespace initialisation in Clojure 1.10.0 are missing the full stacktrace Created: 07/Nov/18  Updated: 13/Nov/18  Resolved: 13/Nov/18

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

Type: Defect Priority: Major
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: regression, stacktrace

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

 Description   

The cause stack is swallowed when exceptions are thrown during initialization of the user namespace. Compiler exceptions thrown after this point have the full stack trace.

The behavior changed between 1.10 alpha6 and alpha7.

➜  ~ cat > /tmp/user.clj
(require 'nonexistent)

➜  ~ clj -Sdeps '{:paths ["/tmp"] :deps {org.clojure/clojure {:mvn/version "1.10.0-alpha6"}}}'
Exception in thread "main" java.lang.ExceptionInInitializerError
	at clojure.main.<clinit>(main.java:20)
Caused by: java.io.FileNotFoundException: Could not locate nonexistent__init.class or nonexistent.clj on classpath., compiling:(user.clj:1:1)
	at clojure.lang.Compiler.load(Compiler.java:7533)
	at clojure.lang.RT.loadResourceScript(RT.java:379)
	at clojure.lang.RT.loadResourceScript(RT.java:366)
	at clojure.lang.RT.maybeLoadResourceScript(RT.java:362)
	at clojure.lang.RT.doInit(RT.java:482)
	at clojure.lang.RT.<clinit>(RT.java:336)
	... 1 more
Caused by: java.io.FileNotFoundException: Could not locate nonexistent__init.class or nonexistent.clj on classpath.
	at clojure.lang.RT.load(RT.java:463)
	at clojure.lang.RT.load(RT.java:426)
	at clojure.core$load$fn__6629.invoke(core.clj:6061)
	at clojure.core$load.invokeStatic(core.clj:6060)
	at clojure.core$load.doInvoke(core.clj:6044)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invokeStatic(core.clj:5863)
	at clojure.core$load_one.invoke(core.clj:5858)
	at clojure.core$load_lib$fn__6574.invoke(core.clj:5903)
	at clojure.core$load_lib.invokeStatic(core.clj:5902)
	at clojure.core$load_lib.doInvoke(core.clj:5883)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invokeStatic(core.clj:659)
	at clojure.core$load_libs.invokeStatic(core.clj:5940)
	at clojure.core$load_libs.doInvoke(core.clj:5924)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invokeStatic(core.clj:659)
	at clojure.core$require.invokeStatic(core.clj:5962)
	at clojure.core$require.doInvoke(core.clj:5962)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at user$eval1.invokeStatic(user.clj:1)
	at user$eval1.invoke(user.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:7069)
	at clojure.lang.Compiler.load(Compiler.java:7521)
	... 6 more

➜  ~ clj -Sdeps '{:paths ["/tmp"] :deps {org.clojure/clojure {:mvn/version "1.10.0-alpha7"}}}'
Exception in thread "main" java.lang.ExceptionInInitializerError
	at clojure.main.<clinit>(main.java:20)

Cause: The changes in CLJ-2373 for CompilerException.toString() use calls to RT. In the scenario above, we are in the path of RT static initialization so RT has not yet been loaded. Thus the toString() logic triggers NoClassDefFoundError while building the message in the toString() of the actual exception.

Approach: Stop using RT.get() in the toString() and use IPersistentMap.valAt() directly.

Patch: clj-2427-2.patch



 Comments   
Comment by Ghadi Shayban [ 08/Nov/18 10:48 AM ]

Thanks Daniel – I edited the repro case a bit.

Comment by Alex Miller [ 12/Nov/18 9:29 PM ]

This is due to the Compiler$CompilerException.toString() change in CLJ-2373, looking at it.

Comment by Alex Miller [ 13/Nov/18 11:45 AM ]

-2 fixes some test failures - oops!

Comment by Daniel Compton [ 13/Nov/18 2:52 PM ]

Fixed in https://github.com/clojure/clojure/commit/d585030f0e12da9351136d6cf886d358942c3b3f.





[CLJ-2136] Reloading multi-arity macros fails Created: 24/Mar/17  Updated: 13/Nov/18  Resolved: 24/Mar/17

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

Type: Defect Priority: Major
Reporter: Joel Kaasinen Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: macro
Environment:

Tested against clojure 1.7 and 1.8



 Description   

When a multi-arity macro is defined using &form and &env, reloading the namespace fails.

Steps to reproduce:

File macro.clj:

(ns macro)

(defmacro macro
  ([x] (macro &form &env x 2))
  ([x y] `(prn ~x ~y)))

REPL:

user=> (require 'macro)
nil
user=> (macro/macro 1)
1 2
nil
user=> (require 'macro :reload)

CompilerException clojure.lang.ArityException: Wrong number of args (4) passed to: macro/macro, compiling:(macro.clj:4:8)

PS. a workaround is to define the macro like

(defmacro macro
  ([x] `(macro ~x 2))
  ([x y] `(prn ~x ~y)))


 Comments   
Comment by Alex Miller [ 24/Mar/17 8:26 AM ]

This macro definition is incorrect as it's passing 4 args from one arity to the other, not 2 (which is what the error tells you). The "workaround" fixes that problem. I don't see a bug here.

Comment by Meloman Cool [ 13/Nov/18 12:45 PM ]

This is clearly a bug, since multi-arity macro definition requires passing two additional arguments (&env and &form), which are usually hidden, to the macro.

See more here: https://stackoverflow.com/a/25569059/6879054

Comment by Nicola Mometto [ 13/Nov/18 12:57 PM ]

No, this is definitely not a bug and the original definition is wrong, as is the stackoverflow answer you posted.

The "workaround" is the only correct way of writing that macro. The original defininition happens to work due to an implementation detail of how macros are implemented in clojure, but is simply incorrect.

Comment by Meloman Cool [ 13/Nov/18 1:05 PM ]

Thank you for clarifying that.





[CLJ-2218] [spec] Improving consistency of explain-data for instrument/macroexpand-check Created: 06/Aug/17  Updated: 11/Nov/18

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

Type: Enhancement Priority: Major
Reporter: Shogo Ohta Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec

Attachments: Text File CLJ-2218.patch    
Patch: Code

 Description   

Description

If you instrument a function, you may get a spec error like the following:

(defn f [x] (inc x))
(s/fdef f
  :args (s/cat :x (s/and integer? even?))
  :ret (s/and integer? odd?))

(t/instrument)

(f 3)
;; ExceptionInfo Call to #'user/f did not conform to spec:
;; In: [0] val: 3 fails at: [:args :x] predicate: even?
;; :clojure.spec.alpha/spec  #object[clojure.spec.alpha$regex_spec_impl$reify__1200
 0x19b3f9a "clojure.spec.alpha$regex_spec_impl$reify__1200@19b3f9a"]
;; :clojure.spec.alpha/value  (3)
;; :clojure.spec.alpha/args  (3)
;; :clojure.spec.alpha/failure  :instrument
;; :clojure.spec.test.alpha/caller  {:file "form-init3240393046310519022.clj", :lin
e 1, :var-scope user/eval1413}
;; clojure.core/ex-info (core.clj:4725)

(ex-data *e) 
;; {:clojure.spec.alpha/problems
;;   [{:path [:args :x],
;;     :pred clojure.core/even?,
;;     :val 3,
;;     :via [],
;;     :in [0]}],
;;  :clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x19b3f9a "clojure.spec.alpha$regex_spec_impl$reify__1200@19b3f9a"],
;;  :clojure.spec.alpha/value (3),
;;  :clojure.spec.alpha/args (3),
;;  :clojure.spec.alpha/failure :instrument,
;;  :clojure.spec.test.alpha/caller {:file "form-init3240393046310519022.clj", :line 1, :var-scope user/eval1413}}

As you can see,

  • the explain-data has a regex (ie. the spec for the args of f) in it as ::s/spec
  • each problem contains :args in their :path

These facts can cause a confusion to spec error reporters because the spec for the args of f ((s/and integer? even?)) has no subspec corresponding to the key :args (I believe :path should only contains keys that is a clue to indicate which subspec to be chosen from a spec).

Possible resolutions

To resolve this confusing situation and improve the consistency of explain-data for instrument check, I think there are two options as follows:

  • Solution 1. removing :args from :path
  • Solution 2. modifying explain-data for instrument check so that they have fspec (rather than :args of it) as ::s/spec

Personally, I prefer Solution 2. since adding fspec in explain-data makes it possible to provide richer error information to *explain-out* implementors.

The same goes for macroexpand-check.



 Comments   
Comment by Alex Miller [ 06/Aug/17 11:14 PM ]

The fspec is the spec in question in here and it does have a component :args (the fspec instance supports key lookup via ILookup for :args as well). So while I would like to improve the error message and data here, I don't agree with removing :args from path. One thing that I think would be useful is for an instrumentation failure to better state the invocation (combining the function and the args into the original call). Right now those are separated and it take some mental work to knit the arg list back together.

Comment by Shogo Ohta [ 07/Aug/17 12:20 AM ]

So while I would like to improve the error message and data here, I don't agree with removing :args from path.

Yes, so once we decide to go with Solution 2., then I think there is no need to remove :args from :path.

One thing that I think would be useful is for an instrumentation failure to better state the invocation (combining the function and the args into the original call).

I totally agree that would be useful, though it sounds like it's somewhat beyond the scope of the ticket in terms of consistency improvement.

Comment by Shogo Ohta [ 24/Aug/17 5:05 AM ]

I've made a patch just to express what I meant. Any feedback will be appreciated.

Comment by Ben Brinckerhoff [ 15/Nov/17 9:02 PM ]

One thing that I think would be useful is for an instrumentation failure to better state the invocation (combining the function and the args into the original call). Right now those are separated and it take some mental work to knit the arg list back together.

Apologies if I'm missing something, but is it even possible to get the function from the explain-data? I don't see it in explain-data, but perhaps it is recoverable somehow? In any case, I would greatly appreciate it if this type of information was available, since it would make it possible to print clearer error messages.

Comment by Shogo Ohta [ 11/Nov/18 10:05 PM ]

Hey, is this already done by CLJ-2392?





[CLJ-2365] Consider integration with java.util.function interfaces Created: 26/Jun/18  Updated: 11/Nov/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: interop

Attachments: Text File some-java-fns-interface.patch    
Approval: Vetted

 Description   

Moving to Java 8 as a baseline allows us to consider built-in ties to critical java.util.Function interfaces like Function, Predicate, Supplier, etc. Needs assessment about what's possible and what automatic integration it would open for users.

https://docs.oracle.com/javase/8/docs/api/java/util/function/package-summary.html



 Comments   
Comment by Jason Whitlark [ 29/Jun/18 5:40 PM ]
;; I dug this out of some scratch code experimenting with kafka streams.  All the reify's were filled with java8 lambdas in the original.

;; I'll dig up another example that shows examples using stuff from java.utils.funstion.* 

;;Some of this was lifted from a franzy example or something?

;; Note that, for example, 
;; https://kafka.apache.org/0102/javadoc/org/apache/kafka/streams/kstream/Predicate.html
;; is different from
;; https://docs.oracle.com/javase/8/docs/api/java/util/function/Predicate.html

(ns utils
  (:import (org.apache.kafka.streams.kstream Reducer KeyValueMapper ValueMapper Predicate))

(defmacro reducer [kv & body]
  `(reify Reducer
     (apply [_# ~(first kv) ~(second kv)]
       ~@body)))

;; public interface KeyValueMapper<K,V,R>
;; apply(K key, V value)
(defmacro kv-mapper [kv & body]
  `(reify KeyValueMapper
     (apply [_# ~(first kv) ~(second kv)]
       ~@body)))

;; public interface ValueMapper<V1,V2>
;; apply(V1 value)
(defmacro v-mapper [v & body]
  `(reify ValueMapper
     (apply [_# ~v]
       ~@body)))

(defmacro pred [kv & body]
  `(reify Predicate
     (test [_# ~(first kv) ~(second kv)]
       ~@body)))

;; I used it something like this:

(ns our-service.kafka-streams
  (:require
   [our-service.util :as k]
   [clojure.string :as str]
  (:import 
           (org.apache.kafka.streams StreamsConfig KafkaStreams KeyValue)
           (org.apache.kafka.streams.kstream KStreamBuilder ValueMapper)))

(defn create-word-count-topology []
  (let [builder (KStreamBuilder.)
        init-stream (.stream builder (into-array ["streams-str-input"]))
        wc (-> init-stream
            (.flatMapValues (k/v-mapper [& value]
                                        (str/split (apply str value) #"\s")))
            (.map (k/kv-mapper [k v]
                               (KeyValue/pair v v)))
            (.filter (k/pred [k v]
                             (println v)
                             (not= v "the")))
            (.groupByKey)
            (.count "CountStore")
            show-item
            ;; this needs to be mapValues
            (.mapValues (reify ValueMapper
                          (apply [_ v]
                            (println v)
                            (str v))))
            (.toStream)
            (.to "wordcount-output"))]
    [builder wc]))
Comment by Ghadi Shayban [ 09/Jul/18 10:30 PM ]

The JLS infers lambda types by hunting for matching functional interfaces AKA "Single Abstract Method" classes [1] (whether they're interfaces or abstract classes.) We could have a reify-like helper that detects these classes [2]. You would have to hint the target class. We don't really need things that are both `IFn` and `j.u.f.Predicate`, etc.

(import '[java.util.function Predicate Consumer])

(let [orig [1 2 3]
      st (atom [])]
  (.forEach orig (jfn Consumer [x] (swap! st conj x)))
  (= @st orig))

[1] https://docs.oracle.com/javase/specs/jls/se8/html/jls-9.html#jls-9.8
[2] spike https://gist.github.com/ghadishayban/0ac41e81d4df02ff176c22d16ee8b972

Comment by Jason Whitlark [ 11/Jul/18 12:26 PM ]

Well, that would be an improvement. The practical problem I run into is that I'm frequently deep in a fluent interface, and don't necessarily know the exact class. That said, it's usually only in a few places. Would it make sense to have a registry? Perhaps something like:

(auto-infer-lambda [java.util.function, org.apache.kafka.streams.kstream])

Comment by Ghadi Shayban [ 11/Jul/18 3:08 PM ]

Do you ever use SAM classes that are abstract classes and not interfaces?

Comment by Andrew Oberstar [ 11/Jul/18 6:10 PM ]

Here's an alternative approach in my ike.cljj library. This uses MethodHandles (i.e. java.lang.invoke package) instead of regular reflection. I'm not sure if I tested this on abstract classes yet.

The usage looks similar to what Ghadi posted:

(defsam my-sam
  java.util.function.Predicate
  [x]
  (= x "it matched"))

(-> (Stream/of "not a match" "it matched")
    (.filter my-sam)
    (.collect Collectors/toList)

(-> (IntStream/range 0 10)
    (.filter (sam* java.util.function.IntPredicate odd?))
    (.collect Collectors/toList)

It uses MethodHandleProxies.asInterfaceInstance to create a proxy instance of the interface that calls a method handle calling a Clojure function. It doesn't try to validate parameter counts, it just treats it as varargs delegating to IFn.applyTo(ISeq). Not sure if it's the most efficient but it was effective for my needs.

I think the LambdaMetaFactory may be the preferred way to meet this type of use case. It was harder for me to follow exactly how to use that though so I didn't end up looking to deep into it.

The main functional issue I have with my approach (and Ghadi's) is that you have to explcitly provide the interface you want to proxy. Java lambdas and Groovy Closures can be used against methods that expect a SAM and it just coerces based on what the method expects. Ideally this would be supported by Clojure too.

Instead of having to do:

(-> (IntStream/range 0 10)
    (.filter (sam* java.util.function.IntPredicate odd?))
    (.collect Collectors/toList)

I'd like to do this:

(-> (IntStream/range 0 10)
    (.filter odd?)
    (.collect Collectors/toList)
Comment by Ghadi Shayban [ 07/Aug/18 2:30 PM ]

Another possible avenue is to extend java.util.function.Supplier to Clojure functions with an explicit 0-arity. That interface is becoming increasingly common in practice; it might be valuable to special-case it. (We shouldn't & couldn't do a similar thing for defrecords, as they already have a method called get, which clashes with Supplier's only method.)

Comment by Marc-André Tremblay [ 22/Sep/18 4:44 PM ]

Moving to Java 8 as a baseline allows us to use default interface methods.

The `some-java-fns-interface.patch` patch implements Consumer, Function, Predicate and Supplier on IFn.

If you want to go this route, I would be very happy to implement all interfaces under java.util.function on IFn as well as the accompanying tests. I currently use this code to play with FoundationDB through its Java client and it works well for me.

https://github.com/marctrem/clojure/commit/97742493f674edd8f6c034ee94da84fa62a76bad

Comment by Jason Whitlark [ 11/Nov/18 4:36 PM ]

If I could just use IFn's anywhere a java.util.function.* was needed, that would be fantastic!





[CLJ-2426] satisfies? doesn't work with the new instance-based protocol polymorphism Created: 06/Nov/18  Updated: 06/Nov/18

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

Type: Defect Priority: Major
Reporter: Juraj Martinka Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: protocols

Approval: Triaged

 Description   

I'd expect that `satisfies?` works even when providing protocol implementation via metadata, however that doesn't seem to be the case:

(defprotocol Foo (foo [x]))
(foo (with-meta [42] {`foo (fn [x] :boo)}))
;; => :boo

;; but `satisfies?` doesn't work
(satisfies? Foo (with-meta [42] {`foo (fn [x] :boo)}))
;; => false





[CLJ-2425] Add Java 11 javadoc url Created: 05/Nov/18  Updated: 06/Nov/18  Resolved: 06/Nov/18

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: interop

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

 Description   

Java 11 has been released since we bumped the javadoc browse urls earlier in Clojure 1.10.

In the process of adding this, I realized it's broken for Java 11 as they now require the module in the url, so should probably fix it for 1.10.

(require '[clojure.java.javadoc :as jd])
(jd/javadoc java.lang.String) ;; should open String javadoc, but currently broken for Java 11 (9 and 10 still work)

Proposed: The attached patch hardcodes the java.base module in the Java 11 javadoc url (covers the most common Java classes, but not all). We could use class.getModule().getName() to build the generic url for all JDK classes, but that would require relying on at least Java 9 APIs to be able to make the call. For now, this does not seem worth doing. When we next bump min JDK we can revisit.

Patch: clj-2425.patch






[CLJ-2424] Fix test case for CLJ-2417 Created: 05/Nov/18  Updated: 06/Nov/18  Resolved: 06/Nov/18

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

Type: Enhancement Priority: Minor
Reporter: Gijs Stuurman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: test

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

 Description   

The test for CLJ-2417 is missing the '(is ..' wrapping in the test, without which the test are run but failures not reported.

This is a separate issue because the patch for CLJ-2417 has already been applied.






[CLJ-2420] Refinement of Clojure error phases and location reporting Created: 31/Oct/18  Updated: 06/Nov/18  Resolved: 06/Nov/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: error-reporting, repl

Attachments: Text File clj-2420-2.patch     Text File clj-2420-3.patch     Text File clj-2420-4.patch     Text File clj-2420-5.patch     Text File clj-2420.patch     Text File error-results.txt    
Patch: Code and Test
Approval: Ok

 Description   

Problems to address:

  • Refine error phases to separate "syntax checking" from the phase itself (in compile and macroexpand)
  • Use phase names that better reflect the actual location of the error (ie :read-source rather than :read)
  • Break ex-str into two parts: ex-triage (semantic analysis of Throwable data) and ex-str (string-making) to increase options for reuse by other tools
  • For execution errors, report the line in the stack reflecting the user operation (outside core), rather than in the guts of core (RT.java, etc)
  • Accept meta on repl forms (:clojure.core/eval-file or :line) and use that as if it was the source file/line.

Patch: clj-2420-5.patch
Screened: clj-2420-5.patch

Changes:

  • Compiler.java
    • CompilerException - adjust phase names
    • remove private isMacroSyntaxCheck() - now covered at point of capture
    • set correct compiler exception phase. currently we bucket all compile errors into :compile-syntax-check. later, we can refine compiler errors and separate some into :compilation. Right now, there is no consistent use of exceptions or way to do this. The code is all ready to work when those change are made.
    • modify eval() to look for and bind file and source-path based on meta :clojure.core/eval-file
  • main.clj
    • removed some commented out dead code
    • added predicate to determine whether stack frame class is a core class
    • added renumbering-read to allow a re-read with line number set from first read
    • removed init-cause (no longer needed)
    • added ex-triage, which takes Throwable->map data and emits an error analysis
    • made ex-str (new in 1.10) take ex-triage data as input and produce a string
    • repl-caught - now uses Throwable->map, ex-triage
    • repl - uses new phase name, removes file setting (moved to Compiler.eval())
  • repl.clj - updated phases to report special message for pst

Also see attached error-results.txt file showing output of a set of examples in each phase. Additional manual tests were run against errors reporting from within files, rather than at repl.



 Comments   
Comment by Alex Miller [ 31/Oct/18 2:31 PM ]

-2 patch adds some new function meta, some doc string changes in ex-triage, and handles the case where the JVM omits the stack.

Comment by Alex Miller [ 01/Nov/18 3:55 PM ]

-3 patch has some fixes in using meta :file and :line annotations

Comment by Alex Miller [ 02/Nov/18 11:59 AM ]

-4 has some more subtle fixes in supporting :file/:line meta





[CLJ-2417] sort and sort-by should retain meta Created: 19/Oct/18  Updated: 06/Nov/18  Resolved: 06/Nov/18

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

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

Attachments: Text File clj-2417-fix-test.patch     Text File clj-2417-fix-test.patch     Text File clj-2417.patch    
Patch: Code and Test
Approval: Ok

 Description   

sort and sort-by don't conceptually change the contents of their source. It would be useful if they retained meta while reordering their contents.

user=> (meta (sort (with-meta (range 10) {:a true})))
nil

Patch: clj-2417.patch



 Comments   
Comment by Ghadi Shayban [ 21/Oct/18 2:49 PM ]

Alex, you may have forgotten the patch

Comment by Alex Miller [ 21/Oct/18 9:12 PM ]

yup

Comment by Gijs Stuurman [ 04/Nov/18 7:49 AM ]

The original patch (clj-2417.patch) is missing the '(is ..' wrapping in the test, without which the test are run but failures not reported. clj-2417-fix-test.patch adds the missing wrapping as a separate patch. Another option is to fix this in the original patch and to re-apply it.

Comment by Alex Miller [ 04/Nov/18 8:35 AM ]

The patch has already been applied, can you make a new ticket?

Comment by Gijs Stuurman [ 05/Nov/18 11:47 AM ]

Test fix as new ticket in https://dev.clojure.org/jira/browse/CLJ-2424





[CLJ-1079] In reader, don't ignore explicit :line :column meta Created: 29/Sep/12  Updated: 06/Nov/18  Resolved: 06/Nov/18

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

Type: Defect Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: reader, repl

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

 Description   

Unfortunately, when read in using a LineNumberingPushbackReader, code like this has its :line metadata squashed by the line numbers coming from that. A REPL-friendly example would be:

=> (meta (read (clojure.lang.LineNumberingPushbackReader.
                 (java.io.StringReader. "^{:line 66} ()"))))
{:line 1, :column 1}
=> (meta (read (java.io.PushbackReader.
                 (java.io.StringReader. "^{:line 66} ()"))))
{:line 66, :column 1}

The latter seems more correct to me (and is equivalent to read-string).

Proposed: Retain existing :line/:column meta on seqs and lists that are read with meta rather than overriding with the values read from the reader.

Additionally, use :file meta as a standard way for the REPL to allow external tool using the repl to specify the source context:

user=> ^{:file "hi.clj" :line 100 :column 10} (def 5)
Syntax error compiling def at (hi.clj:100:10).
First argument to def must be a Symbol

The repl is finding :file meta and binding SOURCE_PATH if it sees it.

Patch: clj-1079-2.patch



 Comments   
Comment by Chas Emerick [ 29/Sep/12 7:07 PM ]

{{CLJ-1097.diff}} contains a fix for this issue, as well as a separate commit that eliminates a series of casts in order to improve readability in the area.

Comment by Andy Fingerhut [ 05/Oct/12 8:23 AM ]

Chas, your patch doesn't apply cleanly to latest Clojure master as of Oct 5 2012. I'm not sure, but I think some recent commits to Clojure on Oct 4 2012 caused that. I also take it as evidence of your awesomeness that you can write patches for tickets that haven't been filed yet

Comment by Chas Emerick [ 05/Oct/12 9:24 AM ]

"patches for tickets that haven't been filed yet?"

Anyway, tweaking up this patch is a small price to pay for having column meta. New {{CLJ-1097.diff}} patch attached, applies clean on master as of now. Otherwise same contents as in the original patch, except:

  • the same dynamic is also applied to :column metadata, now that it's available
  • the changes have been rebased into a single commit (including the elimination of the casts in MetaReader, which were becoming so numerous that the code was less readable than most
Comment by Nicola Mometto [ 05/Oct/12 9:39 AM ]

"patches for tickets that haven't been filed yet?"

He was referring to the fact that you uploaded "CLJ-1097.diff" while the ticket is #1079

Comment by Chas Emerick [ 05/Oct/12 9:42 AM ]

Oh, hah! Twice now, even! One more data point recommending my having slight dyslexia or somesuch. :-P

I've replaced the attached patch with one that is named properly to avoid any later confusion.

Comment by Chas Emerick [ 07/Oct/12 3:57 PM ]

Refreshed patch to apply cleanly to master after the recent off by one patch for :column metadata.

Comment by Stuart Halloway [ 19/Oct/12 3:12 PM ]

This feels backwards to me. If a special purpose tool wants to convey information via metadata, why does it use names that collide with those used by LispReader?

Comment by Chas Emerick [ 19/Oct/12 7:36 PM ]

The information being conveyed is the same :line and :column metadata conveyed by LispReader — in fact, that's where it comes from in the first place.

Kibit (and cljx) is essentially an out-of-band source transformation tool. Given an input like this:

(ns com.foo.hosty)

(defn ^:clj system-hash
  [x]
 (System/identityHashCode x))

(defn ^:cljs system-hash
  [x]
  (goog/getUid x))

…it produces two files, a .clj for Clojure, and a .cljs for ClojureScript. (The first code listing in the ticket description is the former.)

However, because there's no way to transform Clojure code/data without losing formatting, anything that depends on line/column numbers (stack traces, stepping debuggers) is significantly degraded. If LispReader were to defer to :line and :column metadata already available on the loaded forms (there when the two generated files are spit out with *print-meta* on), this would not be the case.

Comment by Andy Fingerhut [ 07/Feb/13 8:47 AM ]

clj-1079-patch-v2.txt dated Feb 7 2013 is identical to Chas's CLJ-1079.diff dated Oct 7 2012, except it applies cleanly to latest master. I believe the only difference is that some white space in the context lines is updated.

Comment by Andy Fingerhut [ 07/Feb/13 12:35 PM ]

Sorry for the noise. I've removed clj-1079-patch-v2.txt mentioned in the previous comment, because I learned that CLJ-1079.diff dated Oct 7 2012 applies cleanly to latest master and passes all tests if you use this command to apply it.

% git am --keep-cr -s --ignore-whitespace < CLJ-1079.diff

I will update the JIRA workflow page instructions for applying patches to mention this, too, because there are multiple patches that fail without --ignore-whitespace, but apply cleanly with that option. That will eliminate the need to update patches merely for whitespace changes.





[CLJ-2423] In ex-info, add ability to set options for suppression and writable stacktrace Created: 05/Nov/18  Updated: 05/Nov/18  Resolved: 05/Nov/18

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

Type: Feature Priority: Minor
Reporter: Alexander Salamatnikov Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: None

Attachments: Text File 2423.patch    

 Description   

There's new Throwable constructor in Java 7 which allows to set options for enabling/disabling suppression and writable stacktrace. The latter is most interesting due to performance reasons: creating Throwable without stacktrace is ~70x faster. Proposed improvement adds ability for ExceptionInfo to accept this options.



 Comments   
Comment by Alex Miller [ 05/Nov/18 8:44 AM ]

Why do we care how fast it is to create a Throwable?

Comment by Alexander Salamatnikov [ 05/Nov/18 8:52 AM ]

I use ExceptionInfo as a "fail" container in https://github.com/dawcs/flow, but I worry a bit about performance, cause creating ExceptionInfo is costly(due to filling stacktrace). In most cases where it's used now, we don't need stacktrace for ExceptionInfo, so I thought it may be good case for optimization.

Comment by Alex Miller [ 05/Nov/18 2:11 PM ]

We use ExceptionInfo stacks all the time so I don't buy that, and I think the case where optimization would affect anything is rare. I'm going to decline this - I don't think it's worth complicating the api for this.





[CLJ-2422] Confusing function name suffixes in error messages Created: 03/Nov/18  Updated: 03/Nov/18

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9, Release 1.10
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: David Bürgin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting, errormsgs
Environment:

Clojure 1.10.0-beta4



 Description   

Some core function names are reported in error messages with an odd munging suffix.

For example, when passing the wrong number of arguments to inc, assoc, and dissoc, each error message reports the function name differently:

clj -Srepro -Sdeps '{:deps {org.clojure/clojure {:mvn/version "1.10.0-beta4"}}}'
user=> (inc)
Syntax error compiling inc at (1:1).
Wrong number of args (0) passed to: clojure.core/inc--inliner--5436
user=> (assoc)
Evaluation error (ArityException) at clojure.lang.AFn.throwArity (AFn.java:429).
Wrong number of args (0) passed to: clojure.core/assoc--5316
user=> (dissoc)
Evaluation error (ArityException) at clojure.lang.AFn.throwArity (AFn.java:429).
Wrong number of args (0) passed to: clojure.core/dissoc

From the user's point of view, this behaviour is unmotivated and confusing. Perhaps such names should be further demunged and printed without the suffix.



 Comments   
Comment by Alex Miller [ 03/Nov/18 1:35 PM ]

The first one here is an inlined function and happens during compilation, so is a little different than the others. Because it's inlined, it's not spec-able. The name parts there are generated within the compiler as well. This one might be fixable based on the regularity of the name construction, although I don't see it as a particularly high priority.

The latter two actually will be a bit different once we apply CLJ-2420:

user=> (assoc)
Execution error (ArityException) at user$eval154/invokeStatic (REPL:1).
Wrong number of args (0) passed to: clojure.core/assoc--5406
user=> (dissoc)
Execution error (ArityException) at user$eval156/invokeStatic (REPL:1).
Wrong number of args (0) passed to: clojure.core/dissoc

Changes here are actually eliding all the internal frames and reporting the top user frame now (here, it happens to be in the REPL invocation, but if it was in a user namespace, you'd see that instead).

I guess in both cases, the numbers you're seeing are a standard technique used throughout Clojure so the question might be whether it's possible to address this in a systematic way. Functions don't inherently carry their unmunged name right now, so demunging from class name is never going to be a foolproof mechanism.

Worth thinking about how to do this better, but not something I expect to look at soon.





[CLJ-1820] Move rename-keys from clojure.set to clojure.core Created: 28/Sep/15  Updated: 02/Nov/18

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

Type: Enhancement Priority: Trivial
Reporter: Lars Andersen Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: None


 Description   

rename-keys is hard to find when it lives in the clojure.set namespace, because it acts on maps and not sets. To my eyes set/rename-keys also looks strange when reading code, but this is the preferred way to bring in the clojure.set namespace.

This is one of the minor warts I'd like to see fixed in clojure 2.0.



 Comments   
Comment by Gordon Syme [ 04/Jul/17 4:56 AM ]

It's not just rename-keys, there are a few fns in clojure.set that don't make sense being there, at first glance anyway.

It certainly harms discoverability of these fns.

I've come across at least one re-implementation of rename-keys and map-invert during my day job because the author didn't know these fns exist.

I'd argue for breaking the relational and map fns out of clojure.set into their own namespaces and deffing some vars in clojure.set for backwards compatibility.
Those compatibility vars could be deleted in 1.10.

I'm happy to do this (or another approach) but would like some buy in on the approach from the core team first.

Comment by Nicola Mometto [ 04/Jul/17 6:25 AM ]

I think this is very, very unlikely to ever happen

Comment by Alex Miller [ 04/Jul/17 11:38 AM ]

We will not delete/move existing vars as this would break existing programs.

Comment by Marc O'Morain [ 12/Jul/17 4:49 AM ]

> We will not delete/move existing vars as this would break existing programs.

This issue can be addressed without deletion as per Gordon's suggestion:

A new var could be added to clojure.core named clojure.core/rename-keys. Then the rename-keys var in clojure.set can be defined as:

(def rename-keys clojure.core/rename-keys)
Comment by Alex Miller [ 12/Jul/17 6:08 AM ]

I am aware of that, which is why I did not close the issue. I was just stating one possible resolution that is off the table.

Comment by Bozhidar Batsov [ 30/Oct/18 4:18 PM ]

Out of curiosity - does anyone know how/why those functions ended up in the `clojure.set` namespace?

P.S. If someone decides to alias things between namespaces I'd suggest creating a `clojure.map` ns over pouring more stuff into core.

Comment by Kevin Downey [ 30/Oct/18 6:28 PM ]

a more accurate name for clojure.set based on its contents would be something like clojure.relational-algebra, where it defines something like relational algebra over sets of maps instead of over sets of tuples. A common operation in that case is renaming keys in maps (similar to using AS in a sql query). Which explains why rename-keys is in clojure.set

Comment by Andy Fingerhut [ 02/Nov/18 12:05 AM ]

Bozhidar - only a guess, not knowledge. Relations are sets of records/tuples, so to me it makes some sense that functions on relations are in clojure.set. rename-keys is probably there because it is used by two of the functions operating on relations.





[CLJ-1975] [spec] clojure.spec attempts to make `empty` records Created: 05/Jul/16  Updated: 01/Nov/18

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

Type: Defect Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: spec
Environment:

1.9.0-alpha11


Approval: Triaged

 Description   
user> (require '[clojure.spec :as s])
nil
user> (defrecord Box [a])
user.Box
user> 
user> (s/conform
        (s/cat :boxes (s/* #(instance? Box %))
               :name (s/coll-of integer?))
        [(Box. 0) [5]])
UnsupportedOperationException Can't create empty: user.Box  user.Box (form-init8049111656025227309.clj:1)
user> (clojure.repl/pst *e)
UnsupportedOperationException Can't create empty: user.Box
       	user.Box (NO_SOURCE_FILE:2)
	clojure.core/empty (core.clj:5151)
	clojure.spec/every-impl/cfns--14008/fn--14014 (spec.clj:1215)
	clojure.spec/every-impl/reify--14027 (spec.clj:1229)
	clojure.spec/conform (spec.clj:150)
	clojure.spec/dt (spec.clj:731)
	clojure.spec/dt (spec.clj:727)
	clojure.spec/deriv (spec.clj:1456)
	clojure.spec/deriv (spec.clj:1463)
	clojure.spec/deriv (spec.clj:1467)
	clojure.spec/re-conform (spec.clj:1589)
	clojure.spec/regex-spec-impl/reify--14267 (spec.clj:1633)

This is a regression from -alpha7; the same sort of spec (modulo the default-value arg to `coll-of`) works as expected there.



 Comments   
Comment by Alex Miller [ 02/Nov/17 3:13 PM ]

In this case, it's considering the s/* to be a non-match and then matching the (Box. 0) against (s/coll-of integer?). This matches the initial predicate (coll?) but falls through to the :else case which presumes it can call `empty`.

You can work around it by adding a :kind predicate to the coll-of:

(s/conform
        (s/cat :boxes (s/* #(instance? Box %))
               :name (s/coll-of integer? :kind #(not (record? %))))
        [(Box. 0) [5]])
;;=> {:boxes [#user.Box{:a 0}], :name [5]}
Comment by Phil Jones [ 19/Jul/18 2:41 PM ]

But why does it need to do empty in the first place?

Does this mean that you can't check collections of records in spec?

Comment by Zach Oakes [ 01/Nov/18 9:22 AM ]

I ran into this myself. Conforming a record to a coll-of spec should show a normal spec error, not throw from deep inside spec's implementation.





[CLJ-2421] Nicer require for stc alias in clojure.spec.test.alpha Created: 31/Oct/18  Updated: 31/Oct/18  Resolved: 31/Oct/18

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

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


 Description   

Right now when I want to alias the namespace clojure.spec.test.check as stc which can be used for passing options to clojure.test.check in clojure.spec.test.alpha/check by using ::stc/opts, I have to do it like this:

(alias 'stc 'clojure.spec.test.check)

If `clojure.spec.test.check` was an empty namespace on the filesystem, I could just do

(ns
  (:require [clojure.spec.test.check :as stc]))

With that change in place, it would also be easier to unify the usage of clojure.spec.test.alpha between Clojure and ClojureScript.

(ns spec-keys-test
  (:require
   [clojure.test :as t :refer [deftest testing is]]
   [clojure.spec.alpha :as s]
   [clojure.test.check]
   [clojure.spec.test.alpha :as stest]
   [clojure.spec.test.check :as stc]
   #?(:cljs [clojure.test.check :as stc])
   ))

#?(:clj (alias 'stc 'clojure.spec.test.check))

could become

(ns spec-keys-test
  (:require
   [clojure.test :as t :refer [deftest testing is]]
   [clojure.spec.alpha :as s]
   [clojure.test.check]
   [clojure.spec.test.alpha :as stest]
   [clojure.spec.test.check :as stc]
   ))

See: https://dev.clojure.org/jira/browse/CLJS-2952



 Comments   
Comment by Alex Miller [ 31/Oct/18 7:11 PM ]

Sorry, not going to do that. You don’t have to alias it at all.





[CLJ-2212] docstring for `defmethod` is imprecise Created: 28/Jul/17  Updated: 31/Oct/18

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

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

Attachments: Text File clj-2212.patch     Text File clj-2212-updated-docstring.patch     Text File clj-2212-updated-params.patch    
Patch: Code
Approval: Triaged

 Description   

arglist:

(defmethod multifn dispatch-val & fn-tail)

docstring:

Creates and installs a new method of multimethod associated with dispatch-value.

There are a few issues with this docstring that make it hard to understand:

1. The & fn-tail argument is not documented. What is the type / shape of & fn-tail. I don't see any mention in the reference guide either - https://clojure.org/reference/multimethods
2. The docstring and arglists conflict about the name multimethod vs. multifn.
3. The docstring and arglists conflict about the name dispatch-value vs. dispatch-val.

Aside: on clojuredocs.org there is a mention of an optional name being allows on the method https://clojuredocs.org/clojure.core/defmethod#example-542692c7c026201cdc3269cd but I don't see that documented anywhere official. I'm not sure if it supported.



 Comments   
Comment by Alex Miller [ 28/Jul/17 9:00 AM ]

fn-tail here means "additional args as could be passed to fn", which does include the function name, so that is supported.

Comment by Marc O'Morain [ 19/Aug/17 3:01 PM ]

Adding patch

Comment by Alex Miller [ 20/Aug/17 5:03 PM ]

Rather than changing the docstring from multimethod to multifn, I would change the code from multifn to multimethod. Otherwise looks good.

Comment by Marc O'Morain [ 22/Oct/18 10:38 AM ]

Adding updated patch that changes the argument name to match the docstring, rather than the other way around.

Comment by Alex Miller [ 22/Oct/18 4:48 PM ]

I'd prefer only the docstring to change here for the dispatch-val (so docstring changing to match the code).

Comment by Marc O'Morain [ 26/Oct/18 11:38 AM ]

Patch that changes only the docstring.

I've not changed the word multimethod to multifn in the docstring. I believe that even though the parameter is not documented, the name multimethod is more clear.

Can you elaborate on why you want to change the docstring rather than the argument name please Alex?

Comment by Alex Miller [ 31/Oct/18 3:57 PM ]

I changed my mind and I think it would be better to just change the docstring to match the arg names.





[CLJ-2419] Multimethods don't have the correct method name in stacktraces. Created: 22/Oct/18  Updated: 23/Oct/18

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

Type: Enhancement Priority: Minor
Reporter: Marc O'Morain Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting, multimethods


 Description   

Describing the user-facing problem:

When an exception is thrown and there is a multimethod in the stacktrace, the name of the multimethod is not shown in the callstack.

When I execute this code:

(defmulti dispatcher (fn [item] (:type item)))

(defmethod dispatcher "cat"
  [{:keys [lives]}]
  (/ lives 0))

(dispatcher {:type "cat" :lives 9})
*e

I get the following stack trace:

[clojure.lang.Numbers divide "Numbers.java" 163]
[clojure.lang.Numbers divide "Numbers.java" 3813]
[config_compilation.core$eval20467$fn__20469 invoke "/Users/marc/dev/circleci/orb-registry/server/src/config_compilation/core.clj" 198]
[clojure.lang.MultiFn invoke "MultiFn.java" 229]
... snip ...

The function name is config_compilation.core$eval20467$fn__20469.

If I use defn to declare dispatcher directly, I get the following stacktrace:

(defn dispatcher
  [{:keys [lives]}]
  (/ lives 0))

(dispatcher {:type "cat" :lives 9})
*e
[clojure.lang.Numbers divide "Numbers.java" 163]
[clojure.lang.Numbers divide "Numbers.java" 3813]
[config_compilation.core$dispatcher invokeStatic "/Users/marc/dev/circleci/orb-registry/server/src/config_compilation/core.clj" 202]
[config_compilation.core$dispatcher invoke "/Users/marc/dev/circleci/orb-registry/server/src/config_compilation/core.clj" 200]
... snip ...

The function name is given correctly as config_compilation.core$dispatcher.

I'm guessing that the issue could be that the defmulti macro not setting the :name metadata, or perhaps not passing the :name metadata to the compiler correctly.



 Comments   
Comment by Alex Miller [ 22/Oct/18 4:43 PM ]

So, it's kind of interesting if you look into the details of this. Technically, at the point where the exception occurs, the defmulti is not on the stack (that's the dispatch function, which has already completed), but the defmethod is.

defmethods are actually defined as fn's and are typically treated as anonymous fns, although it's actually possible to name them independently:

user=> (defmulti dispatcher (fn [item] (:type item)))
#'user/dispatcher
user=> (defmethod dispatcher "cat" cat-method
         [{:keys [lives]}]
         (/ lives 0))
#object[clojure.lang.MultiFn 0x6e1d4137 "clojure.lang.MultiFn@6e1d4137"]
user=> (dispatcher {:type "cat" :lives 9})
ArithmeticException Divide by zero  clojure.lang.Numbers.divide (Numbers.java:163)
user=> (pst *e 5)
ArithmeticException Divide by zero
	clojure.lang.Numbers.divide (Numbers.java:163)
	clojure.lang.Numbers.divide (Numbers.java:3813)
	user/eval155/cat-method--157 (NO_SOURCE_FILE:11)         ;; <== cat-method
	clojure.lang.MultiFn.invoke (MultiFn.java:229)
	user/eval161 (NO_SOURCE_FILE:12)

I think what would be best is if the defmethod function defaulted to a unique name based on the defmulti function name (if no name was specified).

Comment by Alex Miller [ 22/Oct/18 4:45 PM ]

CLJ-2212 is related

Comment by Marc O'Morain [ 23/Oct/18 9:30 AM ]

Oh, interesting.

The symbol that identifies the multimethod (dispatcher) in this case appended with some sort of string representation of the dispatch value would be a really nice default value here.

So you might have functions named
`dispatcher-default`, `dispatcher-cat`, etc.

I assume that you might have to add some additional uniqueness suffix on the end, too.





[CLJ-2418] [spec] (s/exercise string?) produces narrow string samples Created: 20/Oct/18  Updated: 20/Oct/18

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

Type: Enhancement Priority: Minor
Reporter: JAre Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: generative-test, generator, spec, string

Approval: Triaged

 Description   

(s/exercise string?) seems to never produce strings with delimiters or spaces. I noticed this while testing my string formatting function with generative testing - it was extremely surprising.

It looks like a potential source of low quality tests and bad surprises.






[CLJ-1103] Make conj assoc dissoc and transient versions handle args similarly Created: 04/Nov/12  Updated: 11/Oct/18

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

Type: Feature Priority: Minor
Reporter: Andy Fingerhut Assignee: Alex Miller
Resolution: Unresolved Votes: 10
Labels: collections, transient

Attachments: Text File clj-1103-10.patch     File clj-1103-7.diff     Text File clj-1103-8.patch     Text File clj-1103-9.patch    
Approval: Triaged

 Description   

Examples that work as expected:

Clojure 1.7.0-master-SNAPSHOT
user=> (dissoc {})
{}
user=> (disj #{})
#{}
user=> (conj {})
{}
user=> (conj [])
[]

Examples that do not work as desired, but are changed by the proposed patch:

user=> (assoc {})
ArityException Wrong number of args (1) passed to: core/assoc  clojure.lang.AFn.throwArity (AFn.java:429)
user=> (assoc! (transient {}))
ArityException Wrong number of args (1) passed to: core/assoc!  clojure.lang.AFn.throwArity (AFn.java:429)
user=> (dissoc! (transient {}))
ArityException Wrong number of args (1) passed to: core/dissoc!  clojure.lang.AFn.throwArity (AFn.java:429)

;; patch enables conj! with multiple arguments, like conj
user=> (conj! (transient []) 1 2 3)
ArityException Wrong number of args (4) passed to: core/conj!  clojure.lang.AFn.throwArity (AFn.java:429)

;; patch would give error for missing value for last key to assoc!, not silently use nil for last value
user=> (assoc! (transient {}) 1 2 3)
#object[clojure.lang.PersistentArrayMap$TransientArrayMap 0x2e7569b8 "clojure.lang.PersistentArrayMap$TransientArrayMap@2e7569b8"]

I looked through the rest of the code for similar cases, and found that there were some other differences between them in how different numbers of arguments were handled, such as:

+ conj handles an arbitrary number of arguments, but conj! does not.
+ assoc checks for a final key with no value specified (CLJ-1052), but assoc! did not.

History/discussion: A discussion came up in the Clojure Google group about conj giving an error when taking only a coll as an argument, as opposed to disj which works for this case:

https://groups.google.com/forum/?fromgroups=#!topic/clojure/Z9mFxsTYTqQ

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 04/Nov/12 6:04 PM ]

clj-1103-make-conj-assoc-dissoc-handle-args-similarly-v1.txt dated Nov 4 2012 makes conj conj! assoc assoc! dissoc dissoc! handle args similarly to each other.

Comment by Brandon Bloom [ 09/Dec/12 5:30 PM ]

I too ran into this and started an additional discussion here: https://groups.google.com/d/topic/clojure-dev/wL5hllfhw4M/discussion

In particular, I don't buy the argument that (into coll xs) is sufficient, since into implies conj and there isn't an terse and idiomatic way to write (into map (parition 2 keyvals))

So +1 from me

Comment by Andy Fingerhut [ 08/Nov/13 10:41 AM ]

Patch clj-1103-2.diff is identical to the previous patch clj-1103-make-conj-assoc-dissoc-handle-args-similarly-v1.txt except it applies cleanly to latest master. The only changes were some changed context lines in a test file.

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

Patch clj-1103-3.diff is identical to the patch clj-1103-2.diff, except it applies cleanly to latest master. The only changes were some doc strings for assoc! and dissoc! in the context lines of the patch.

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

Patch clj-1103-4.diff is identical to the previous clj-1103-3.diff, except it updates some context lines so that it applies cleanly to latest Clojure master as of today.

Comment by Alex Miller [ 05/Jun/14 9:29 PM ]

Can someone update the description with some code examples? Or drop them here at least?

Comment by Brandon Bloom [ 05/Jun/14 9:35 PM ]

What do you mean code examples?

These currently work as expected:
(dissoc {})
(disj #{})

The following fail with arity errors:
(assoc {})
(conj {})

Similarly for the transient ! versions.

This is annoying if you ever try to do (apply assoc m keyvals)... it works at first glance, but then one day, bamn! Runtime error because you tried to give it an empty sequence of keyvals.

Comment by Andy Fingerhut [ 06/Aug/14 5:05 PM ]

Patch clj-1103-5.diff dated Aug 6 2014 applies cleanly to latest Clojure master as of today, whereas the previous patch did not. Rich added 1-arg version of conj to 1.7.0-alpha1, so that change no longer is part of this patch.

Comment by Andy Fingerhut [ 29/Aug/14 6:04 PM ]

Patch clj-1103-6.diff dated Aug 29 2014 is identical to the former patch clj-1103-5.diff (which will be deleted), except it applies cleanly to the latest Clojure master.

Comment by Andy Fingerhut [ 26/May/15 9:00 PM ]

Patch clj-1103-7.diff dated May 25 2015 is identical to the former patch clj-1103-6.diff (which will be deleted), except it applies cleanly to the latest Clojure master.

Comment by Michael Blume [ 12/Oct/15 5:32 PM ]

Updated patch to apply cleanly to master

Comment by Michael Blume [ 06/Oct/17 4:16 PM ]

Updated patch

Comment by Andy Fingerhut [ 06/Oct/17 6:00 PM ]

Is it intentional that clj-1103-9.patch leaves out the changes to assoc! that were in the earlier patches? I can create another updated one if so – just curious if there was a reason for the change.

Comment by Michael Blume [ 06/Oct/17 7:17 PM ]

That was not intentional at all, thanks for catching it, I'm honestly not sure what happened on my end to make that happen. I think this patch should be a correct update. Sorry about that.

Comment by Andy Fingerhut [ 09/Oct/17 10:17 PM ]

No worries, and I thank you for keeping the patch current, even while keeping my name on it.





[CLJ-2123] Lighter-weight aliasing for keywords Created: 10/Mar/17  Updated: 11/Oct/18

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

Type: Feature Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 30
Labels: keywords, namespace

Approval: Vetted

 Description   

It is useful to make qualified keywords, and particularly so with spec. Using namespace aliases helps a lot in working with a lot of qualified keywords. However, currently creating an aliased namespace requires that the namespace actually exists.

This ticket is a placeholder to do something more with lighter-weight aliasing for keywords. Details TBD.



 Comments   
Comment by Herwig Hochleitner [ 26/Sep/17 8:01 PM ]

Hoping to get the conversation started on this in time for a 1.9-beta:

The rationale in my predecessor ticket, CLJ-2030, was to create a construct to allow for auto-aliasing in .cljc files, which I would like to submit as a requirement. The obvious place for cljs to declare an auto-alias is the ns clause.
So if we don't want to grow alias for that use case, I would propose to grow the ns clause with a declaration for keyword - aliases. Let's give it a working title of (:kwns-alias ..) for this comment.

:kwns-alias would be used to establish namespace aliases for ::qualified/keywords

One open question is, how :kwns-alias should interact with alias. i.e. whether the namespace of ::qualified/keyword should always expand to the same as the one of a qualified/symbol or if they should be allowed to differ. I'd argue they should always be the same, because of the rule of simplicity. That means, that

  • alias will need to check if the sym is already in :kwns-alias and throw, if so
  • :kwns-alias will need to also work on `qualified/keywords probably shouldn't have knws in its name any more

So what's a good name for :kwns-alias? :let maybe?

Comment by Alex Miller [ 26/Sep/17 8:23 PM ]

beta = feature complete, so this isn't happening in 1.9.

I think adding anything to ns is likely off the table, but until I learn more about what Rich has in mind, I can't really suggest anything more.

Comment by Herwig Hochleitner [ 27/Sep/17 7:05 AM ]

that's a pity. but maybe 1.10 will have a shorter release time ...

In any case, I'll be very interested in Rich's idea to do this outside of the ns clause and still have it fit well with clojurescript (or even clojure, for that matter, ...)
Do you have any rationale, why growing ns for this is a bad idea?

Comment by Alex Miller [ 27/Sep/17 7:07 AM ]

ns already does way too much stuff and we don't have any desire to make it do more.

Comment by Herwig Hochleitner [ 27/Sep/17 12:20 PM ]

Since ns is essential to how a namespace is set up in clojure, I'd imagine a fix for that problem (of ns doing too much) to imply some rather drastic changes to the state of the art. As we won't break ns, I'd imagine either some sort of ns2 or a way to do namespace setup outside of the ns clause. I'm curious either way.

Since I know, that Rich will not be hassled about his hammock time, I'll lay off this for now. I'll be sure to check back as the next round of alphas goes out. I also hope for some productive discussion in the meantime.

Comment by Tom Locke [ 11/Oct/18 4:51 AM ]

Is there any reason such a mechanism would be limited to keywords? I have a place in my code where it would be very useful to have such a feature for symbols.

Also, related: a way for pr-str (and similar) to be configured to use short versions of namespaces. Debug output can be awfully noisy, even with namespaced maps.

Comment by Alex Miller [ 11/Oct/18 10:29 AM ]

No particular reason - usually for stuff like this we make it work the same for any ident (keyword or symbol).





[CLJ-2415] Error cause should always be on 2nd line of error message Created: 08/Oct/18  Updated: 09/Oct/18  Resolved: 09/Oct/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: error-reporting, errormsgs

Attachments: Text File clj-2415-2.patch     Text File clj-2415-3.patch     Text File clj-2415-4.patch     Text File clj-2415-5.patch     Text File clj-2415.patch    
Patch: Code
Approval: Ok

 Description   

After the changes in 1.10 alphas, the cause is sometimes, but not always on the 2nd line of the error message. This should be standardized so the cause message is always on the 2nd line.

Before:

Clojure 1.10.0-beta1
user=> :::5
Syntax error reading source at (2:0). Cause: Invalid token: :::5

user=> (let [x])
Syntax error macroexpanding clojure.core/let at (2:1). Cause: Call to clojure.core/let did not conform to spec.
[x] - failed: even-number-of-forms? at: [:bindings] spec: :clojure.core.specs.alpha/bindings

user=> (cond 1)
Syntax error macroexpanding cond at (3:1).
Cause: cond requires an even number of forms

user=> (defmulti 5 class)
Unexpected error macroexpanding defmulti at (4:1).
Cause: ClassCastException class java.lang.Long cannot be cast to class clojure.lang.IObj (java.lang.Long is in module java.base of loader 'bootstrap'; clojure.lang.IObj is in unnamed module of loader 'app')

user=> (def 5)
Syntax error compiling def at (5:1).
Cause: First argument to def must be a Symbol

user=> (/ 1 0)
Evaluation error at clojure.lang.Numbers.divide (Numbers.java:188). ArithmeticException Divide by zero

user=> (+ 1 :a)
Evaluation error at clojure.lang.Numbers.add (Numbers.java:153). ClassCastException class clojure.lang.Keyword cannot be cast to class java.lang.Number (clojure.lang.Keyword is in unnamed module of loader 'app'; java.lang.Number is in module java.base of loader 'bootstrap')

user=> (assert false)
Evaluation error at user/eval140 (NO_SOURCE_FILE:8). AssertionError Assert failed: false

;; Requires a foo.clj with:
;;  (defn x [a] a)  (defn y [a] (x a))  (s/fdef x :args (s/cat :a keyword?))  
user=> (require '[clojure.spec.alpha :as s] '[clojure.spec.test.alpha :as stest] 'foo)
user=> (stest/instrument 'foo/x)
user=> (foo/y 10)
Evaluation error at clojure.spec.test.alpha/spec-checking-fn/conform!--3024 (alpha.clj:132). ExceptionInfo Call to #'foo/x did not conform to spec.
10 - failed: keyword? at: [:a]

After:

user=> :::5
Syntax error reading source at (2:0). 
Invalid token: :::5

user=> (let [x])
Syntax error macroexpanding clojure.core/let at (2:1).
[x] - failed: even-number-of-forms? at: [:bindings] spec: :clojure.core.specs.alpha/bindings

user=> (cond 1)
Syntax error macroexpanding cond at (3:1).
cond requires an even number of forms

user=> (defmulti 5 class)
Unexpected error macroexpanding defmulti at (4:1).  
java.lang.Long cannot be cast to class clojure.lang.IObj 

user=> (def 5)
Syntax error compiling def at (5:1).
First argument to def must be a Symbol

user=> (/ 1 0)
Evaluation error (ArithmeticException) at clojure.lang.Numbers.divide (Numbers.java:188). 
Divide by zero

user=> (+ 1 :a)
Evaluation error (ClassCastException) at clojure.lang.Numbers.add (Numbers.java:153). 
clojure.lang.Keyword cannot be cast to class java.lang.Number 

user=> (assert false)
Evaluation error (AssertionError) at user/eval140 (NO_SOURCE_FILE:8). 
Assert failed: false

user=> (require '[clojure.spec.alpha :as s] '[clojure.spec.test.alpha :as stest] 'foo)
user=> (stest/instrument 'foo/x)
user=> (foo/y 10)
Evaluation error - invalid arguments to foo/x at (foo.clj:8).
10 - failed: keyword? at: [:a]

Patch: clj-2415-5.patch

Changes (in order they occur in the patch):

  • For :read errors, move cause to 2nd line and remove "Cause: " at beginning
  • For :print errors, put exception class inside 1st line and move cause to 2nd line
  • For eval spec errors, use new message that includes fn and caller location
  • For eval non-spec errors, move exception class into 1st line and message on 2nd line
  • For macroexpand spec errors, omit spec ex-info msg which duplicates
  • For macroexpand non-spec errors, move message onto 2nd line and remove "Cause:"
  • For compile errors, move exception to 2nd line and remove "Cause:"





[CLJ-2414] Regression in reflectively finding default methods Created: 07/Oct/18  Updated: 09/Oct/18  Resolved: 09/Oct/18

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

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

Java 9+


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

 Description   

CLJ-2066 introduced a new reflective walk for matching accessible methods but the logic in that walk assumes it will find a method in the superclass hierarchy. If it does not, then it will produce an infinite loop.

This is an example of a concrete class (sun.nio.fs.UnixPath) which has a method startsWith which is declared as a default method on java.nio.file.Path:

(def p (java.nio.file.Paths/get "src" (into-array String [])))
(.startsWith p "s")

Proposed: Instead, should walk all super classes, then walk all interfaces to find an accessible default method.

Patch: clj-2414-2.patch






[CLJ-2416] [core.specs] defmulti and defmethod spec Created: 09/Oct/18  Updated: 09/Oct/18

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: core.specs

Attachments: Text File clj-2416.patch    
Patch: Code
Approval: Vetted

 Description   

Currently defmulti and defmethod don't have specs and don't have much macro validation.

user=> (defmulti 5 class)
ClassCastException class java.lang.Long cannot be cast to class clojure.lang.IObj

Add specs for defmulti and defmethod.

After:

user=> (defmulti 5 class)
Syntax error macroexpanding clojure.core/defmulti at (1:1).
5 - failed: simple-symbol? at: [:fn-name]

Patch: clj-2416.patch






[CLJ-2165] #clojure/var tag for transmitting var identity Created: 22/May/17  Updated: 08/Oct/18

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

Type: Feature Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: print, reader, var

Attachments: Text File vartag2.patch    
Approval: Vetted

 Description   

Currently one can't send vars around in edn. #' is clojure reader specific. Objective is to transmit var identity and bind to same-named var on reading side (a la var serialization support).

Proposed: This is not generic enough to add to edn, so use #clojure/var for tag. Printing may print #clojure/var instead of #' (perhaps via a flag) - needs more assessment. #clojure/var tag reader should be installed in data readers.

Patch: vartag2.patch



 Comments   
Comment by Christophe Grand [ 11/Jul/17 10:14 AM ]

Should unnamed vars (eg created by with-local-vars) print to #clojure/var nil or throw an exception? (exception is the print-dup behavior)

Comment by Steven Yi [ 08/Aug/17 11:49 AM ]

I think the vartag2.patch has an issue in that the test for print-var-tagged in missing an assertion. I think it is supposed to have something like:

(is (and ...))

within the last let-binding.

Comment by Ghadi Shayban [ 17/Mar/18 2:21 PM ]

Printing unnamed vars has little utility (nothing distinguishes them from each other without knowing their code context), seems like it would be fine to have #clojure/var nil or #clojure/var :unnamed





[CLJ-2112] [spec] Add specs for spec forms Created: 15/Feb/17  Updated: 08/Oct/18

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

Type: Feature Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 23
Labels: spec

Attachments: Text File spec-forms.patch    
Approval: Incomplete

 Description   

It would be useful to have specs that described spec forms, such that it was possible to go from a spec form like (s/keys :req [::a ::b] :opt [::c]) to a conformed version that allowed you to grab the parts without parsing the s-expression. This can be done by creating specs, thus allowing:

user=> (require '[clojure.spec :as s] '[clojure.spec.specs])
user=> (s/def ::aspec (s/keys :req [::a ::b] :opt [::c]))
user=> (def aspec-data (s/conform :clojure.spec.specs/spec (s/form ::aspec)))
user=> (pr aspec-data)
[:form {:s clojure.spec/keys, 
        :args {:req [[:key :clojure.spec.specs/a] [:key :clojure.spec.specs/b]], 
               :opt [:clojure.spec.specs/c]}}]
user=> (map val (-> aspec-data val :args :req))
(:clojure.spec.specs/a :clojure.spec.specs/b)

Patch: spec-forms.patch (a work in progress)



 Comments   
Comment by Saul Shanabrook [ 05/Aug/17 6:08 PM ]

Could I help out on this? Happy to work on it. It would be very helpful for me, trying parse the the specs from the :args and :ret in fspec.

Comment by Alex Miller [ 06/Aug/17 11:05 PM ]

As you can see, there is an existing patch here with substantial work on it already (and some early review from Rich and Stu). The most useful help on this at the moment would be feedback on the gaps/open questions in it.

Comment by Alex Miller [ 06/Aug/17 11:17 PM ]

Also, I should mention that I do not expect this to be finalized and included until we have finalized the spec forms themselves as they are still subject to change.

Comment by Alexander Kiel [ 27/Jun/18 7:34 AM ]

In the current patch, keywords referencing specs are missing except for keys in `s/keys`. What would the right way to model the keywords? One way would be to add `qualified-keyword?` as fourth option to the or-spec of `::spec`. Than `::spec` could be used as spec for the spec arg in `s/valid?`, `s/conform`, `s/form` and others.

Comment by Alex Miller [ 27/Jun/18 8:07 AM ]

Yeah, that would probably be good.

Comment by Alexander Kiel [ 27/Jun/18 10:45 AM ]

Another observation: Currently s/and and s/or are allowed to contain no preds:

(defspec clojure.spec.alpha/and (s/* ::spec))
(defspec clojure.spec.alpha/or (s/* (s/cat :tag keyword? :pred ::spec)))

there should be either a clear semantic definition what an empty form will mean or we should just use s/+ here and require at least one pred. For the and-form, we could define that (s/and) is the same as (s/and any?) and the same as just any?. For the or-form it is difficult, because s/conform returns tagged results. So conforming any value with (s/or) will result in ::s/invalid which renders an empty or-form useless.

Comment by Alex Miller [ 27/Jun/18 10:54 AM ]

There is a clear semantic definition for these - they are the same as clojure.core/and and clojure.core/or. As far as I'm concerned, this is all correct.

Comment by Alexander Kiel [ 28/Jun/18 7:33 AM ]

Thanks. I see. The documentation in clojure.core defines that (clojure.core/and) returns true and (clojure.core/or) returns nil.





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

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

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

OS X


Attachments: File clj-1400-2.diff     File clj-1400-3.diff     File clj-1400-4.diff    
Patch: Code and Test
Approval: Incomplete

 Description   

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

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

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

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

Patch: clj-1400-4.diff

Screened by: Alex Miller



 Comments   
Comment by Scott Bale [ 25/Jun/14 9:58 AM ]

This looks to me like relatively low hanging fruit unless I'm missing something; assigning to myself.

Comment by Scott Bale [ 26/Jun/14 11:23 PM ]

Patch clj-1400-1.diff to Compiler.java.

With this patch the example would now look like:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

I'm not sure the if(namesStaticMember(sym)) [see below], and the 2nd branch, is even necessary. Just by inspection I suspect it is not.

[footnote]

public static boolean namesStaticMember(Symbol sym){
	return sym.ns != null && namespaceFor(sym) == null;
}
Comment by Scott Bale [ 26/Jun/14 11:24 PM ]

patch: code and test

Comment by Scott Bale [ 26/Jun/14 11:27 PM ]

I tested on an actual source file, and the exception message included the file/line/col info as desired:

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)
Comment by Andy Fingerhut [ 29/Aug/14 4:46 PM ]

Patch clj-1400-1.diff dated Jun 26 2014 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch. See section "Updating Stale Patches" on this wiki page for some tips on updating patches: http://dev.clojure.org/display/community/Developing+Patches

Comment by Scott Bale [ 31/Aug/14 3:53 PM ]

Attached is an updated patch: "clj-1400-2.diff". I removed the stale patch.

Comment by Alex Miller [ 09/Sep/14 9:29 AM ]

Few comments to address:

  • Compiler diff was using spaces, not tabs, which makes it harder to diff. I attached a -3.diff that fixes this.
  • the call to namesStaticMember seems weird. The name of that method is confusing for this use. Beyond that, I think it's doing more than you need. That method is going to attempt resolve the qualified name in terms of the current ns, but I think you don't even want to do that. Rather you just want to know if the sym has a ns (sym.ns != null) - isn't that enough?
  • In what case will the other error "Var doesn't exist" occur? In other words, in what case will lookupVar not succeed in creating a new var here? If there is no such case, then remove this case. If there is such a case, then add a test.
Comment by Scott Bale [ 11/Sep/14 11:19 PM ]

Agree with all three of your bullets. Attached is an updated patch, clj-1400-4.diff.

  • I used tabs in Compiler.java
  • After close inspection of call to lookupVar(...), I believe null is returned only in the case of exactly this ticket (the symbol having a non-null namespace which has not been loaded yet). So I've taken out the conditional and the 2nd branch.
  • (Test is unchanged)
Comment by Scott Bale [ 11/Sep/14 11:22 PM ]

(properly named patch)

Comment by Alex Miller [ 11/Sep/14 11:37 PM ]

You could throw a CompilerException with the location of the problem instead (as the ticket description suggests).

Comment by Scott Bale [ 19/Sep/14 2:37 PM ]

Sorry, I should've mentioned because this wasn't obvious to me either (and in fact I forgot until just now): the RuntimeException is already caught and wrapped in a CompilerException.

I'm not sure which try-catch block within Compiler.java this is happening in, there are multiple. But you can see in the output that the exception is a CompilerException and the file|line|col info is there:

In the Repl...

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

...or in a source file

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)

Also, at the point at which the RuntimeException of this patch is being thrown, the source line and col params to CompilerException are not available, or at least not afaict.

Comment by Alex Miller [ 07/Oct/14 12:34 PM ]

I'll follow up on this patch later - Rich thought it was making too many assumptions. I think we validated many of those but need to double-check those.





[CLJ-1095] Allow map-indexed to accept multiple collections (a la map) Created: 25/Oct/12  Updated: 08/Oct/18

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

Type: Feature Priority: Trivial
Reporter: Bo Jeanes Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File 0001-map-indexed-accepts-multiple-collections.patch     Text File 0002-Add-test-for-multi-collection-map-indexed-fn.patch     Text File CLJ-1601 map-indexed and tests 8-5-2017.patch    
Approval: Vetted

 Description   

Bring external interface of map-indexed in line with map. Existing usages of map-indexed unchanged both in implementation and interface.

examples
(map vector (range 10 20) (range 30 35)) ;=> ([10 30] [11 31] [12 32] [13 33] [14 34])
(map-indexed vector (range 10 20) (range 30 35)) ;=> ([0 10 30] [1 11 31] [2 12 32] [3 13 33] [4 14 34])

Patch: CLJ-1601 map-indexed and tests 8-5-2017.patch - also adds tests for map-indexed

Prescreened by: Alex Miller



 Comments   
Comment by Aaron Bedra [ 25/Oct/12 5:11 PM ]

Can you add a test for the improved functionality?

Comment by Bo Jeanes [ 25/Oct/12 5:20 PM ]

You bet. I tried to before submitting this but found no existing tests for map-indexed to expand upon. Given that, I decided to just start the conversation first. If you think this is a good addition, I'll find a place to stick the tests and add a new patch file.

Comment by Bo Jeanes [ 25/Oct/12 8:05 PM ]

Add two unit tests for map-indexed. One tests old behavior (single collection) and the second tests mapping across 3 collections.

There were no existing tests for map-indexed that I could see to expand upon (using git grep map-indexed src/clojure)

Comment by Justin Spedding [ 31/Jul/17 9:38 PM ]

This feature would be very useful. Is there a reason that no one continued to work on this in almost 5 years?

Comment by Alex Miller [ 31/Jul/17 10:32 PM ]

Predates my involvement with Clojure so don't know but don't see any reason why not.

Comment by Andy Fingerhut [ 31/Jul/17 10:36 PM ]

If a ticket has not been moved to the Vetted state by Rich Hickey, it is unknown whether it is of interest to be added to Clojure at all. The default answer to any change to Clojure, until there is a compelling argument to the core Clojure developers otherwise, is "no". That said, JIRA tickets tend to be left open rather than declined if there is no compelling argument for declining it.

For some more background, see here, in particular the "Clojure Governance and How It Got That Way" article linked from there: https://dev.clojure.org/display/community/Contributing+FAQ

For details on Vetted, and other JIRA ticket statuses used by the Clojure core team, see here: https://dev.clojure.org/display/community/JIRA+workflow

Voting on tickets (there is a "Vote" link near the top right of the page if you log in to your JIRA account first) can in some cases help get attention to them sooner than tickets without votes, but there are no guarantees on time lines.

Comment by Alex Miller [ 31/Jul/17 10:43 PM ]

I didn't try but I'm sure this patch no longer applies. Having a single combined patch that can be applied would be a useful first step.

Comment by Justin Spedding [ 05/Aug/17 7:40 PM ]

I just uploaded a new patch for map-indexed that allows it to accept multiple collections. I took the exact same approach that map does, but with the addition of the index. It has better performance than the old patch from 2012. I also included more tests. Both the function update and the new tests are in "CLJ-1601 map-indexed and tests 8-5-2017.patch"

Also, map-indexed had 0 tests prior to me writing the patch. That seems a bit dangerous for a function in core to be untested.

Comment by Justin Spedding [ 04/Oct/17 11:37 AM ]

It's been a little while since I uploaded the patch here and it was prescreened. Both this ticket and another that I created are awaiting vetting, and the FAQ says that I should tend to my tickets. Is there something else that I should be doing to tend to it and move it along?

Comment by Alex Miller [ 04/Oct/17 2:37 PM ]

I don't think we'll consider any more enhancements in 1.9 timeframe.





[CLJ-2368] [spec] describe* of spec-impl returns nil :args and :fn Created: 29/Jun/18  Updated: 08/Oct/18

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

Type: Defect Priority: Minor
Reporter: Alexander Kiel Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File clj-2368.patch    
Patch: Code
Approval: Vetted

 Description   

The form of an empty fspec returns nil at :args and :fn:

(s/form (s/fspec))
=> (clojure.spec.alpha/fspec :args nil :ret clojure.core/any? :fn nil)

The problem is, that the specs from CLJ-2112 don't validate that form. I suggest to implement describe* of fspec-impl like that of map-spec-impl which omits nil values.

Patch: clj-2368.patch



 Comments   
Comment by Alex Miller [ 29/Jun/18 8:55 AM ]

Sounds right.





[CLJ-2360] [spec] Inconsistent results from s/valid? and s/explain when using s/or without parameters Created: 19/Jun/18  Updated: 08/Oct/18

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

Type: Defect Priority: Minor
Reporter: Björn Ebbinghaus Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec
Environment:

org.clojure/spec.alpha "0.1.143"


Attachments: Text File clj-2360.patch    
Patch: Code and Test
Approval: Vetted

 Description   

When using spec/or without parameters spec/valid? and spec/explain are resulting in a contradiction.

Minimal example:

(s/valid? (s/or) :something)
=> false
(s/explain (s/or) :something)
Success!
=> nil

Similar to or, s/or should fail to validate anything (so first call is good) but should report an error with explain (so explain result is bad).

Proposed: In the s/or explain, the logic maps over all preds to build a problem list about each value not matching, but the base case of no preds isn't explicitly handled. The change in the code checks for that case and builds a specific explain problem error for the overall s/or not matching. Patch includes fix and tests for (s/or) and (s/and) (which is ok).

Patch: clj-2360.patch






[CLJ-2286] Update `clj` REPL with hint: (use Crtl-D to exit) Created: 11/Dec/17  Updated: 08/Oct/18

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

Type: Enhancement Priority: Minor
Reporter: Alan Thompson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: repl
Environment:

All


Attachments: Text File clj-2286.patch    
Patch: Code
Approval: Vetted

 Description   

Update clojure.main REPL for the `clj` tool with a hint for terminating the session via Ctrl-D, like so (changes in bold):

~ > clj
Clojure 1.9.0 (use Ctrl-D to exit)
user=>
~ >

The lein repl accepts `exit`, `quit`, and Crtl-D to exit; the `clj` repl accepts only Crtl-D. A small hint upon startup on the proper way to terminate the repl session will smooth the experience for users expecting for their old habits to work.

Patch: clj-2286.patch






[CLJ-2168] [spec] clojure.spec: :pred in explain for coll-of should have resolved symbols Created: 26/May/17  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: spec
Environment:

0.1.123


Attachments: Text File clj-2168.patch    
Patch: Code and Test
Approval: Vetted

 Description   

:pred should be resolved in explain problems like:

s/coll-of and s/every-kv should have resolved :pred functions if it's values aren't valid:

(::s/problems (s/explain-data (s/coll-of (fn [x] (pos? x))) [-1]))
({:path [], :pred (fn [x] (pos? x)), :val -1, :via [], :in [0]})

should be

(::s/problems (s/explain-data (s/coll-of (fn [x] (pos? x))) [-1]))
({:path [], :pred (clojure.core/fn [x] (clojure.core/pos? x)), :val -1, :via [], :in [0]})

Other examples:

;; same with every
(::s/problems (s/explain-data (s/every (fn [x] (pos? x))) [-1]))
({:path [], :pred (fn [x] (pos? x)), :val -1, :via [], :in [0]})

;; :distinct option pred is not resolved:
(::s/problems (s/explain-data (s/coll-of pos? :distinct true) [-1 -1]))
[{:path [], :pred distinct?, :val [-1 -1], :via [], :in []}]

map-of and every-kv do not have this issue. The :count, :min-count, :max-count, and :kind options do correctly produce resolved :preds.

Patch: clj-2168.patch



 Comments   
Comment by Shogo Ohta [ 31/May/17 2:19 AM ]

The same problem happens with s/every.

Comment by Shogo Ohta [ 01/Jun/17 9:02 PM ]

Oh, sorry. I meant s/every-kv, not s/every.

BTW, after looking into things around this, I found some other spec macros were putting inconsistent forms of :pred in their explain data.

For example,

(s/explain-data (s/tuple integer?) []) => (clojure.core/= (clojure.core/count %) 1)
(s/explain-data (s/& integer? even?) []) => #function[clojure.core/integer?] (not a symbol)

I'll file that as another ticket later.

Comment by Alexander Kiel [ 18/Jul/18 7:56 AM ]

Issue https://github.com/alexanderkiel/phrase/issues/22 relates to this.





[CLJ-2143] [spec] The result of s/form for s/keys* is different from the original form Created: 05/Apr/17  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Shogo Ohta Assignee: Alex Miller
Resolution: Unresolved Votes: 2
Labels: spec

Approval: Vetted

 Description   

If s/form is applied to s/keys*, it returns a value completely different from the original form:

user=> (s/form (s/keys* :req-un [::x ::y]))
(clojure.spec/& (clojure.spec/* (clojure.spec/cat :clojure.spec/k clojure.core/keyword? :clojure.spec/v clojure.core/any?)) :clojure.spec/kvs->map mspec__14270__auto__)
user=>


 Comments   
Comment by Alex Miller [ 05/Apr/17 8:57 AM ]

Thanks for logging - I've been working on an approach for this one but never got around to actually logging it.





[CLJ-2036] [spec] Generators for some? and any? only return collections or nil Created: 07/Oct/16  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: generator, spec

Approval: Vetted

 Description   

Both of these should generate a better variety of values (strings, keywords, symbols, numbers) in addition to collections and nil.



 Comments   
Comment by Sean Corfield [ 07/Oct/16 11:55 AM ]

See also http://dev.clojure.org/jira/browse/TCHECK-111 which may solve this directly?

Comment by Gary Fredericks [ 09/Oct/16 2:08 PM ]

This is probably http://dev.clojure.org/jira/browse/TCHECK-83, which is fixed on test.check master.

Comment by Alex Miller [ 11/Oct/16 12:14 PM ]

I'm going to leave this open pending a new release and dependency update to test.check, which I presume will address it.





[CLJ-1908] Add clojure.test api to run single test with fixtures and report Created: 01/Apr/16  Updated: 08/Oct/18

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

Type: Feature Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: test

Attachments: Text File CLJ-1908-3.patch    
Approval: Vetted

 Description   

When developing code, it is sometimes effective to focus on a single failing test, rather than running all tests in a namespace. This can be the case when running the tests takes some amount of time, or when running the tests produces a large volume of failures. The best option for running a single test with fixtures currently is `test-vars` ala:

(use 'clojure.test) 
(def counter (atom 0)) 
(defn setup [f] (swap! counter inc) (f)) ;; a :once fixture with state 
(use-fixtures :once setup) 
(deftest ex (println "counter =" @counter))

(test-vars [#'ex])  ;=> counter = 1 
(test-vars [#'ex])  ;=> counter = 2

However, this has the following issues:

  • No test reporting feedback such as you get with run-tests (on success, there is no output)
  • Need to specify var (not symbols) wrapped in a vector

Proposed: A new macro `run-test` that specifies a single symbol and does the same test reporting you get with `run-tests`. Usage:

(use 'clojure.test) 
(def counter (atom 0)) 
(defn setup [f] (swap! counter inc) (f)) ;; a :once fixture with state 
(use-fixtures :once setup) 
(deftest ex (println "counter =" @counter)) 

(run-test ex)

;=> Testing user
;=> counter = 1

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

(run-test ex)

;=> Testing user
;=> counter = 2

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

Patch: CLJ-1908-3.patch



 Comments   
Comment by Howard Lewis Ship [ 01/Apr/16 4:12 PM ]

Having trouble with the patch, in that, things that work at the REPL fail when executed via `mvn test`. Tracking down why is taking some time.

Comment by Howard Lewis Ship [ 01/Apr/16 4:40 PM ]

Initial patch; code works but mvn test fails and I haven't figured out why.

Comment by Howard Lewis Ship [ 01/Apr/16 5:44 PM ]

Thanks to Hiredman, was provided with insight that back ticks needed due to how Maven/Ant runs the tests. All tests now pass.

Comment by Alex Miller [ 01/Apr/16 6:43 PM ]

As far as I can tell, this is basically the same intent as CLJ-866 which was completed in Clojure 1.6. You can do this now with test-vars:

user=> (use 'clojure.test) 
nil 
user=> (def counter (atom 0)) 
#'user/counter 
user=> (defn setup [f] (swap! counter inc) (f)) ;; a :once fixture with state 
#'user/setup user=> (use-fixtures :once setup) {:clojure.test/once-fixtures (#object[user$setup 0x7106e68e "user$setup@7106e68e"])} user=> (deftest ex (println "counter =" @counter)) #'user/ex user=> (test-vars [#'ex]) 
counter = 1 
nil 
user=> (test-vars [#'ex]) 
counter = 2 
nil
Comment by Howard Lewis Ship [ 03/Apr/16 12:27 PM ]

I think there is some advantage to being able to run the tests using is symbol, not its var. Further, the change I've suggested also returns the same kind of data that `run-tests` does.

Comment by Alex Miller [ 04/Apr/16 9:23 AM ]

Some changes needed on this patch before I will prescreen it:

  • Patch should be squashed to a single commit
  • Commit message in patch should start with "CLJ-1908"
  • Change run-test* to run-test-var
  • The docstring for run-test-var should be: "Run the test in Var v with fixtures and report." Kill the "called from" sentence".
  • The first sentence of the docstring for run-test should be: "Runs a single test in the current namespace." Remove "This is meant to be invoked interactively, from a REPL.". Last sentence is ok.
  • In run-test, replace (ns-resolve ns test-symbol) with the simpler (resolve test-symbol).
Comment by Howard Lewis Ship [ 04/Apr/16 10:52 AM ]

Thanks for the input; I'll have an updated patch shortly.

Comment by Howard Lewis Ship [ 08/Apr/16 2:51 PM ]

Updated patch, squashed and reflecting all of Alex's comments.





[CLJ-1905] loop should retain primitive int or float without widening Created: 29/Mar/16  Updated: 08/Oct/18

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

Type: Enhancement Priority: Minor
Reporter: Renzo Borgatti Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: compiler, performance, primitives
Environment:

Possibly older Clojure versions (but not verified).


Attachments: Text File 0001-CLJ-1905-remove-useless-widening-on-loop-bindings.patch    
Patch: Code and Test
Approval: Vetted

 Description   

In the following example:

(defn incrementer [n]
  (let [n (int n)]
    (loop [i (int 0)]
      (if (< i n)
        (recur (unchecked-inc-int i))
        i))))

the loop-local starts as an int when just a local but is widened to a long in the recur. It should be possible to retain the primitive int (or alternately float) type on the recur, rather than widening to long (or double).

The compiler code that is promoting the int seems to be:
https://github.com/clojure/clojure/blob/a19c36927598677c32099dabd0fdb9d3097df259/src/jvm/clojure/lang/Compiler.java#L6377-L6380

Proposed: remove useless widening on loop bindings

Patch: 0001-CLJ-1905-remove-useless-widening-on-loop-bindings.patch

Prescreening comments: My main question here is: do we want to support primitive int/float loop vars?



 Comments   
Comment by Alex Miller [ 29/Mar/16 10:54 AM ]

I don't think anything but primitive longs or doubles are intended to be supported in loop/recur. Presuming that is correct, this would be the expected behavior.

The last major round of numeric/primitive refactoring made the decision to focus only on supporting primitive longs and doubles. One consequence of this is that primitive int loops are difficult to optimize - the main time I run into this is when working with Java interop in a tight loop on (for example) String, collection, or array operations (all of which are int-indexed).

Re unchecked-inc vs unchecked-inc-int, the primary reason to have these two variants is not performance but behavior. In particular, hashing operations often expect to get 32-bit int overflow semantics, not 64-bit int overflow semantics.

In summary, I think in the example given I would not write it with either int or unchecked-inc-int but with long and unchecked-inc if you are looking for best performance.

Comment by Nicola Mometto [ 29/Mar/16 11:01 AM ]

Alex Miller I don't think that's correct, as (AFAIK) it's only in fn params/returns that primitive types are supposed to be restricted to longs and doubles.
Note that for example, char, byte, boolean, short etc work just fine in both let and loop, while int and float work fine in let but not in loop.

This is caused by the following 4 lines in Compiler.java https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L6278-L6281

As far as I can tell, there's no reason for those 4 lines to be there at this point in time, and removing them makes int and float locals to be emitted correctly inside loops

This example in clojure.org itself seems to assume that ints should work in loops http://clojure.org/reference/java_interop#primitives

Also from that same paragraph:

All Java primitive types are supported
let/loop-bound locals can be of primitive types, having the inferred, possibly primitive type of their init-form

Comment by Alex Miller [ 29/Mar/16 12:07 PM ]

I agree that it should be possible to let-bound primitives of other types - I'm really talking about what happens on recur.

What would you expect to happen for a fn recur target? I wouldn't expect primitives other than long or double to work there since they can't occur in the function signature.

Note that I haven't closed this ticket, still talking through this.

Comment by Alex Miller [ 29/Mar/16 12:10 PM ]

I've definitely run into cases where creating a primitive int loop/recur would be useful for tight interop loops given how common int-indexing is in Java (some of the alioth benchmarks in particular would benefit from this). I think the argument is far weaker for float though.

Comment by Nicola Mometto [ 29/Mar/16 12:19 PM ]

I don't think we need to worry about fn recur targets at all, given that the only possible primitive bindings there are either long or double, and int/floats would get widened anyway, but good point, the tests in a patch for this ticket need to be sure that case is indeed handled.

RE: floats – I recall people complaining about bad support for floats when using clojure for graphical processing.

Even if admittedly a weak argument, I'm always of the idea that we should strike to be as consistent as possible. I don't think anybody would expect let/loop locals to behave differently, or differences between primitive types (other than the documented limitation about long/double being the only working prim types for fn arguments/return vals)

Comment by Alex Miller [ 29/Mar/16 12:30 PM ]

I'll leave this one in here but I'm going to treat it as an enhancement to current behavior. I think there's a good chance that Rich will just say this is working as intended.

I don't think the example is a very good one though and would welcome a better example. The reservations regarding unchecked-inc-int do not seem correct or valid to me (as usage should be fine on longs and is not designed for perf reasons anyways). A good example would should usage of a Java api in a loop where int-indexing and int-math gives better performance.

Comment by Nicola Mometto [ 30/Mar/16 8:51 AM ]

I edited the title as the bug is in `loop`, not `recur`

Comment by Nicola Mometto [ 02/Apr/16 9:55 AM ]

Attached a patch that removes the useless widenings done by the compiler on loop bindings, here's a benchmark demonstrating the speedup gained when areducing over int-arrays:

Before patch:

Clojure 1.8.0
user=> (use 'criterium.core)
nil
user=> (let [xs (int-array (range 1e6))] (bench (areduce xs i ret 0 (+ ret (aget xs i)))))
Evaluation count : 64260 in 60 samples of 1071 calls.
             Execution time mean : 954.009929 µs
    Execution time std-deviation : 20.292809 µs
   Execution time lower quantile : 926.331747 µs ( 2.5%)
   Execution time upper quantile : 1.009189 ms (97.5%)
                   Overhead used : 1.840681 ns

Found 4 outliers in 60 samples (6.6667 %)
	low-severe	 4 (6.6667 %)
 Variance from outliers : 9.4244 % Variance is slightly inflated by outliers
nil

After patch:

Clojure 1.9.0-master-SNAPSHOT
user=> (use 'criterium.core)
nil
user=> (let [xs (int-array (range 1e6))] (bench (areduce xs i ret 0 (+ ret (aget xs i)))))
Evaluation count : 68640 in 60 samples of 1144 calls.
             Execution time mean : 870.462532 µs
    Execution time std-deviation : 13.100790 µs
   Execution time lower quantile : 852.357513 µs ( 2.5%)
   Execution time upper quantile : 896.531529 µs (97.5%)
                   Overhead used : 1.844045 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil




[CLJ-1895] Remove loading of clojure.string in clojure.java.io Created: 22/Feb/16  Updated: 08/Oct/18

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

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

Attachments: Text File clj-1895.patch    
Patch: Code
Approval: Vetted

 Description   

clojure.core loads clojure.java.io to define slurp and spit. clojure.java.io loads clojure.string, solely for a single call to replace. This slows down Clojure core startup for no reason.

Approach: Replace clojure.string/replace call with a Java interop call to .replace. This saves about 18 ms during Clojure core startup.

Patch: clj-1895.patch






[CLJ-1771] Support for multiple key(s)-value pairs in assoc-in Created: 29/Jun/15  Updated: 08/Oct/18

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

Type: Feature Priority: Major
Reporter: Griffin Smith Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: None
Environment:

All


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

 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

Patch: clj-1771.patch

Prescreened by: Alex Miller



 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.

Comment by Yehonathan Sharvit [ 19/Aug/16 10:19 AM ]

for the sake of symmetry with `assoc` I'd love to see this ticket fixed

Comment by Alex Miller [ 27/Nov/16 10:33 PM ]

Do you need the "if kvs" check?

Should have tests.

Comment by Matthew Gilliard [ 14/Jan/17 11:34 AM ]

Sorry for the delay - I don't get notifications from this JIRA for some reason.

The patch now includes tests.

Both `if` checks are necessary as we have 3 possible outcomes there:
1/ No more kvs (we are finished)
2/ More kvs (we need to recur)
3/ A sequence of keys but no value (throw IAE)





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

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

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

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

 Description   

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

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

Steps to reproduce:

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

(ns leak.main)

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

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

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

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

(def unrelated 42)

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

Patch: clj-1620-v5.patch



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

Patch from Nicola Mometto

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

Attached the same patch with a more informative better commit message

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Ok, looks like the minimal case is

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

(ns bar (:require [foo]))

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

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

Comment by Michael Blume [ 06/Jan/15 11:30 PM ]

Well, not a minimal case, of course, but one without lein, at least.

Comment by Michael Blume [ 06/Jan/15 11:51 PM ]

ok, problem is instaparse's defclone macro, I've extracted it to a test repo

https://github.com/MichaelBlume/thunk-fail

lein do clean, compile will get you a failure, but the repo has no dependencies so I'm sure there's a way to do that without lein.

Comment by Ghadi Shayban [ 06/Jan/15 11:56 PM ]

Sorry for the barrage of questions, but these classloader bugs are subtle (and close to being solved I hope). Your report is immensely valuable, and yet it will help to be even more specific. There are a cluster of these bugs – and keeping them laser-focused is key.

The minimal case to which you refer is the NoSuchFieldError?
How are is this being invoked this without lein?
What are you calling to AOT? (compile 'bar) ?
What is the classpath? When you invoke originally, is ./target/classes empty?
Does the problem go away with CLJ-979-7 applied?

Comment by Michael Blume [ 07/Jan/15 12:16 AM ]

I have tried and failed to replicate without leiningen. When I just run

java -Dclojure.compile.path=target -cp src:../clojure/target/clojure-1.7.0-aot-SNAPSHOT.jar clojure.lang.Compile thunk-fail.first thunk-fail.second

everything works fine.

Comment by Ghadi Shayban [ 07/Jan/15 12:30 AM ]

The NoSuchFieldError is related to the keyword lookup sites.

Replacing defclone's body with
`(do (:foo {})) is enough to trigger it, with the same ns structure.

Comment by Nicola Mometto [ 07/Jan/15 4:47 AM ]

I have updated the patch for CLJ-1544, now the combination of the new patch + the patch from this ticket should not cause any exception.

That said, a bug in this patch still exists since while the patch for CLJ-1544 had a bug, it was causing a perfectly valid (albeit hardly reproducible) compilation scenario so we should keep debugging this patch with the help of the bugged patch for CLJ-1544.

I guess the first thing to do is figure out what lein compile is doing differently than clojure.Compile

Comment by Nicola Mometto [ 07/Jan/15 4:49 AM ]

Also Ghadi is right, infact replacing the whole body of thunk-fail.core with (:foo {}) is enough.

It would seem like the issue is with AOT (re)compiling top-level keyword lookup sites, my guess is that for some reason this patch is preventing correct generation of the __init static initializer.

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

I still have absolutely no idea what lein compile is doing but I figured out the issue.
The updated patch binds (in eval) the appropriate vars only when already bounded.

Comment by Alex Miller [ 07/Jan/15 9:00 AM ]

Would it be worth using transients on the bindings map now?

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

Makes sense, updated the patch to use a transient map

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

Is there a test we can add that'll fail in the presence of the v2 patch? preferably independent of the CLJ-1544 patch? I can try to write one myself, but I don't have a lot of familiarity with the Clojure compiler internals.

Comment by Nicola Mometto [ 07/Jan/15 12:32 PM ]

I'll have to think about a way to reproduce that bug, it's not a simple scenario to reproduce.
It involves compiling a namespace from an evaluated context.

Comment by Laurent Petit [ 15/Apr/15 11:14 AM ]

Hello, is there any chance left that this issue will make it to 1.7 ?

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

Wasn't planning on it - what's the impact for you?

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

The impact is that I need to use a patched version of Clojure for CCW.
While it's currently not that hard to follow clojure's main branch and regularly rebase on it or reapply the patch, it's still a waste of time.

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

I will check with Rich whether it can be screened for 1.7 before we get to RC.

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

same as v4 patch, but just has more diff context

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

the file mentioned in the patch field is not the right one IMHO

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

which one is?

Comment by Laurent Petit [ 01/May/15 8:58 AM ]

I think you previous comment relates to clj-1620-v5.patch, but at the end of the description there's the following line:

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

Comment by Alex Miller [ 01/May/15 9:30 AM ]

Those patches are equivalent with respect to the change they introduce; they just differ in how much diff context they have.

Comment by Alex Miller [ 18/May/15 2:25 PM ]

Rich has ok'ed screening this one for 1.7 but I do not feel that I can mark it screened without understanding it much better than I do. The description, code, and cause information here is not sufficient for me to understand what the problem actually is or why the fix is the right one. The fix seems to address the symptom but I worry that it is just a symptom and that a better understanding of the actual cause would lead to a different or better fix.

The evolution of the patches was driven by bugs in CLJ-1544 (a patch which has been pulled out for being suspect for other reasons). Starting fresh, were those modifications necessary and correct?

Why does this set of vars need to push clean impls into the bindings? Why not some of the other vars (like those pushed in load())? The set chosen here seems to match that from the ReifyParser - why? Why should they only be pushed if they are bound (that is, why is "not bound" not the same as "bound but empty")? Are we affecting performance?

Popping all the way out, is the thing being done by CCW even a thing that should be doable? The description says "Compiling a function that references a non loaded (or uninitialized) class triggers its init static" - should this load even happen? Can we get an example that actually demonstrates what CCW was doing originally?

Comment by Laurent Petit [ 19/May/15 7:12 AM ]

Alex, the question of "should what CCW is doing be doable" can be answered if you answer it on the given example, I think.

The question "should the initialization of the class occur when it could just be loaded" is a good one. Several reports have been made on the Clojure list about this problem, and I guess there is at least one CLJ issue about changing some more classForName into classForNameNonLoading here and there in Clojure.
For instance, it prevents referencing java classes which have code in their static initializers as soon as the code does some supposition about the runtime it is initialized in. This is a problem with Eclipse / SWT, this a problem with Cursive as I remember Colin mentioning a similar issue. And will probably is a problem that can appear each time one tries to AOT compile clojure code interoperating with java classes who happen to have, somewhere within static initializers triggered by the compilation (and this is transitive), assumptions that they are initialized in the proper target runtime environment.

What I don't know is if preventing the initialization to occur in the first place would be sufficient to get rid of the class of problems this bug and the proposed patch tried to solve. I do not claim to totally what is happening either (Christophe and Nicolas were of great help to analyze the issue and create the patch), but as I understand it, it's a kind of "Inception-the-movie-like" bug. Compiling a fn which triggers compiling another fn (here through the loading of clojure namespaces via a java initializer).

If preventing the initialization of class static methods when they are referenced (through interop calls - constructor, field, method, static field, static method-) is the last remaining bit that could cause such "compilation during compilation" scenario, then yes, protecting the compilation process like Nicolas tried to do may not be necessary, and just fixing the undesired loading may be enough.





[CLJ-1435] 'numerator and 'denominator fail to handle integral values (i.e. N/1) Created: 30/May/14  Updated: 08/Oct/18

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

Type: Enhancement Priority: Major
Reporter: Aaron Brooks Assignee: Unassigned
Resolution: Unresolved Votes: 11
Labels: math

Attachments: Text File clj-1435.patch    
Patch: Code and Test
Approval: Incomplete

 Description   

Because ratio values reduce to lowest terms and, for integral values where the lowest term is N/1, are auto-converted to BigInts (and formerly Longs), the current behavior of clojure.core/numerator and clojure.core/denominator yield unexpected results.

user=> (numerator 1/3)
1
user=> (numerator (+ 1/3 2/3))

ClassCastException clojure.lang.BigInt cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3306)
user=> (denominator 1/3)
3
user=> (denominator (+ 1/3 2/3))

ClassCastException clojure.lang.BigInt cannot be cast to clojure.lang.Ratio  clojure.core/denominator (core.clj:3314)
user=>

It's confusing to not support numerator and denominator on integer types as this requires you to always check ratio? before invoking them.

Proposed: Extend numerator and denominator to also work on integer types (long, BigInt, BigInteger) by routing to overloaded methods on Numbers for the desired types.

Patch: clj-1435.patch

Screened by: Alex Miller

Questions from screening for Rich:

1. numerator and denominator are tagged as returning java.math.BigInteger (not clojure.lang.BigInt) and that's what I followed in the patch. Seems like maybe that should be BigInt though? Not sure on what basis to make that decision.
2. Should numerator and denominator accept both BigInteger and BigInt?



 Comments   
Comment by Andy Fingerhut [ 30/May/14 6:35 PM ]

I don't know the official stance on this ticket, but will add some notes.

Aaron, numerator and denominator are pretty clearly documented to work on Ratio types only.

It is pretty easy to write my-numerator and my-denominator that work exactly as you wish, checking for the type of arg and using numerator, denominator for Ratio types, and doing whatever you think is correct for other numeric types.

Comment by Aaron Brooks [ 30/May/14 7:44 PM ]

I'm aware that they are documented as such. Part of my point is that you can be working entirely with Ratio types and, via arithmetic operations between them, sometimes wind up with a non-Ratio number unexpectedly.

Also consider:

user=> (numerator 2/1)
ClassCastException java.lang.Long cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3238)

You're then left either implementing a try/catch correction or always checking the type before using numerator or denominator which is a loss in performance.

The patch I have in mind is creating a protocol, extended to Ratio, BigInt and Long which calls the appropriate method (Ratios) or returns either the given number or 1 (numerator/denominator) for the integral types. I expect this to maintain the current level of performance in the cases where it works and behave properly in the cases currently not handled.

Comment by Gary Fredericks [ 27/Aug/15 10:38 AM ]

I've definitely written the helper functions Andy describes on several occasions.

Comment by Felipe Micaroni Lalli [ 01/Sep/15 4:58 PM ]

Related issue: https://stackoverflow.com/questions/25194809/how-to-convert-any-number-to-a-clojure-lang-ratio-type-in-clojure

A workaround to that is (numerator (clojure.lang.Numbers/toRatio (rationalize <put any type of number here>)))

Comment by Alex Miller [ 04/Nov/16 9:16 AM ]

I agree with the intent of the ticket here that these should work. I'm not sure about the protocol approach as that would be an open system and I'm not sure that's what we want. An alternative would be to just create Java methods on Numbers that took the appropriate types and let the JVM sort it out.

Comment by Aaron Brooks [ 08/Apr/17 12:38 PM ]

Any progress on this?

Comment by Alex Miller [ 08/Apr/17 1:28 PM ]

Sitting in the queue, waiting for love.

Comment by Aaron Brooks [ 08/Apr/17 3:54 PM ]

Can I give it love? If there's a direction that we can agree on, I'd be happy to create the patch.

Comment by Alex Miller [ 08/Apr/17 6:13 PM ]

I've screeened the patch that's here already?

Comment by Andy Fingerhut [ 27/Sep/18 4:32 AM ]

The new versions of numerator and denominator after this patch was applied perform reflective calls. Is that expected?

Comment by Andy Fingerhut [ 27/Sep/18 5:06 AM ]

Created ticket CLJ-2408 to make the question more prominent than a comment on a closed ticket.

Comment by Alex Miller [ 05/Oct/18 10:58 PM ]

Patch reverted





[CLJ-1364] Primitive VecSeq does not implement equals or hashing methods Created: 19/Feb/14  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections, ft

Attachments: Text File clj-1364.patch    
Patch: Code and Test
Approval: Vetted

 Description   

VecSeq (as produced by (seq (vector-of :int 1 2 3))) does not implements equals, hashCode, or hasheq and does not play with any other Clojure collections or sequences appropriately in this regard.

user=> (def rs (range 3))
user=> (def vs (seq (vector-of :int 0 1 2)))
user=> rs
(0 1 2)
user=> vs
(0 1 2)
user=> (.equals rs vs)
true
user=> (.equals vs rs)    ;; expect: true
false
user=> (.equiv rs vs)
true
user=> (.equiv vs rs)
true
user=> (.hashCode rs)
29824
user=> (.hashCode vs)     ;; expect to match (.hashCode rs)
2081327893
user=> (System/identityHashCode vs)  ;; show that we're just getting Object hashCode
2081327893
user=> (.hasheq rs)
29824
user=> (.hasheq vs)       ;; expect same as (.hasheq rs) but not implemented at all
IllegalArgumentException No matching field found: hasheq for class clojure.core.VecSeq  clojure.lang.Reflector.getInstanceField (Reflector.java:271)

Approach: Implement Object.hashCode(), Object.equals(), and IHashEq.hasheq() in the primitive vector seq implementation. All of these leverage the prim vec seq itself rather than the underlying prim vec as it was quite a big simpler. The hasheq() impl calls Murmur3/hashOrdered, which takes an Iterable, so Iterable was also implemented using an iterator over the seq.

Some existing tests were expanded to include coverage of the primitive vec seq.

Patch: clj-1364.patch



 Comments   
Comment by Andy Fingerhut [ 09/May/18 1:53 PM ]

Maybe this is a separate issue, but `(.equals (vector-of :long 0 1 2) (range 3))` is also false (i.e. without calling seq on the return value of vector-of).

Comment by Alex Miller [ 09/May/18 1:55 PM ]

Same issue, and is fixed by the patch.

Comment by Andy Fingerhut [ 09/May/18 2:23 PM ]

Excellent. Sorry, I had not read the new tests in the patch carefully enough to see that this is covered there. Good to know.





[CLJ-1360] Doc that clojure.string/split strips trailing delimiters Created: 18/Feb/14  Updated: 08/Oct/18

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

Type: Defect Priority: Minor
Reporter: Tim McCormack Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: docstring, string

Attachments: Text File clj-1360-2.patch     Text File clj-1360.patch    
Patch: Code
Approval: Vetted

 Description   

clojure.string/split and clojure.string/split-lines inherit the bizarre default behavior of java.lang.String#split(String,int) in stripping trailing consecutive delimiters:

(clojure.string/split "banana" #"an")
⇒ ["b" "" "a"]
(clojure.string/split "banana" #"na")
⇒ ["ba"]
(clojure.string/split "nanabanana" #"na")
⇒ ["" "" "ba"]

In the case of split-lines, processing a file line by line and rejoining results in truncation of trailing newlines in the file. In both cases, the behavior is surprising and cannot be inferred from the docstrings. A workaround for split is to pass a limit of -1.

Proposed: As current users may be relying on the current behavior, the attached merely updates the docstring to warn of this behavior and suggest use of -1 as a limit to workaround.

Patch: clj-1360-2.patch



 Comments   
Comment by Andy Fingerhut [ 18/Feb/14 10:51 AM ]

Probably documenting would be safer than changing the behavior at this point, given that some people may actually rely on the current behavior after testing, deploying, etc.

I don't currently have a suggestion for a modified doc string, but note that there are examples of this behavior and how one can use an extra "-1" limit argument at the end to get all split strings: http://clojuredocs.org/clojure_core/clojure.string/split

Comment by Crispin Wellington [ 21/May/15 10:46 PM ]

This bug just bit me. +1 to be fixed. If we just document and leave the behavior as is, then we have a surprising and inconsistent behaving split (why are inner empty values kept, but outer ones stripped?) that is different to every other split you've ever used. The optional -1 limit argument looks hacky but a fix could keep this -1 argument working.

EDIT: this looks to be java's string class behavior: http://stackoverflow.com/questions/2170557/split-method-of-string-class-does-not-include-trailing-empty-strings
Would be nice if limit defaulted to -1 on that type of clojure.string/split call.

Comment by Stuart Halloway [ 19/Jul/15 8:03 AM ]

This is really gross, and the original developer has been punched in the neck. (Ow.)

I hate the Java leakage, but given that this is already out there, and that people are likely already relying on both the default and the negative-arg behavior, I think the least bad bet is to document precisely the semantics we have.

Comment by Alex Miller [ 29/Mar/18 3:45 PM ]

Incorporate changes from CLJ-1857 in clj-1360-2.patch.





[CLJ-1180] defprotocol doesn't resolve tag classnames Created: 10/Mar/13  Updated: 08/Oct/18

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

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

Attachments: Text File 001-CLJ-1180.patch     Text File clj-1180-2.patch    
Patch: Code and Test
Approval: Vetted

 Description   

defprotocol doesn't resolve tag classnames, this results in exceptions being thrown when the declared protocol uses as a tag an imported class that is not imported in the namespace that uses it.

user=> (import 'clojure.lang.ISeq)
clojure.lang.ISeq
user=> (defprotocol p (^ISeq f [_]))
p
user=> (ns x)
nil
x=> (defn x [y] (let [z (user/f y)] (inc z)))
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: ISeq, compiling:(NO_SOURCE_PATH:4:33)

Patch: clj-1180-2.patch

Screened by: Alex Miller

  • My only hesitancy with this patch is making the tagToClass public and reaching into it from core_deftype - this means that call into the Compiler needs to remain open forever as an api.


 Comments   
Comment by Alex Miller [ 03/Sep/13 9:38 AM ]

Similer to CLJ-1232.

Comment by Alex Miller [ 18/Jan/16 4:50 PM ]

Patch updates for current master, retains attribution.





[CLJ-2199] [spec] Error attempting to unform unconformed keys (no :conform-keys opt) Created: 05/Jul/17  Updated: 08/Oct/18

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

Type: Defect Priority: Minor
Reporter: Daniel Stockton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File clj-2199.patch     Text File clj-2199.patch    
Patch: Code
Approval: Vetted

 Description   

Minimal failing case:

(s/def ::key-spec (s/or :kw keyword? :str string?))
(s/def ::map-spec (s/map-of ::key-spec identity))
(println (s/unform ::map-spec (s/conform ::map-spec {:a :b})))
java.lang.UnsupportedOperationException: nth not supported on this type: Keyword

If keys are not conformed, we should also not attempt to unform them.



 Comments   
Comment by Daniel Stockton [ 08/Aug/17 8:29 AM ]

Add test and fix behaviour

Comment by Daniel Stockton [ 08/Aug/17 8:39 AM ]

Actually, although this passes all tests, it's not alright because it bypasses validation.

Comment by Daniel Stockton [ 12/Aug/17 4:23 AM ]

I think it passes for correctness this time but open to advice if this is not a good approach.





[CLJ-2190] [spec] clojure.spec.alpha/exercise-fn should emit a bettor error message when no implementation is found for a symbol Created: 27/Jun/17  Updated: 08/Oct/18

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

Type: Enhancement Priority: Minor
Reporter: Abhirag Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, spec
Environment:

Clojure 1.9.0-alpha17
test.check 0.10.0-alpha2


Attachments: Text File clj-2190.patch    
Patch: Code
Approval: Vetted

 Description   

Here we get a NullPointerException because although we do have a spec for foo, we don't have an implementation for it, a more descriptive error message would help.

(require '[clojure.spec.alpha :as s])

(s/fdef foo :args (s/cat :x int?) :ret int?)
=> user/foo

(s/exercise-fn `foo)
NullPointerException   clojure.core/apply (core.clj:657)

Proposed: Check for a nil function and throw.

Patch: clj-2190.patch



 Comments   
Comment by Stuart Halloway [ 22/Jun/18 3:20 PM ]

If we are going to handle this case, can we also include the name of the offending symbol in the error message?

Comment by Alex Miller [ 22/Jun/18 3:49 PM ]

The sym-or-f arg is resolved to nil before exercise-fn is invoked, so we have no symbol to report by that point.





[CLJ-2102] [spec] Reduce collection generator default size from 20 Created: 25/Jan/17  Updated: 08/Oct/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: generator, spec

Attachments: Text File clj-2102-2.patch     Text File clj-2102-3.patch     Text File clj-2102.patch    
Patch: Code
Approval: Incomplete

 Description   

In general I find that it is very easy (especially with nested or recursive collections) to have a check run OOME due to generating very large nested collections. Currently the default is 20 - I think we should change it to 3.

The attached patch just changes the default from 20 to 3. An alternate approach would be to change it to a dynvar setting.

Patch: clj-2102-3.patch



 Comments   
Comment by Alex Miller [ 07/Apr/17 3:45 PM ]

Updated patch to apply to master

Comment by Alex Miller [ 10/May/17 12:54 PM ]

Updated patch to apply to spec.alpha

Comment by Stuart Halloway [ 12/May/17 10:32 AM ]

I have definitely seen the pain here – nested collections can get big fast. OTOH for non-nested collections the larger generator is nice. Not sure moving the default is a help.

Comment by Alex Miller [ 12/May/17 3:23 PM ]

Use dynamic var and reduce default. Also consider ways to avoid this kind of problem in test.check itself (how does quickcheck deal with this?).





[CLJ-1952] include var->sym in clojure.core Created: 08/Jun/16  Updated: 08/Oct/18

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

Type: Feature Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File clj-1952.patch     Text File clj-1952-v2.patch    
Approval: Vetted

 Description   

A lot of libraries define their own variant of `var->sym`, clojure.spec recently did so aswell as a private var called `->sym`.

This ticket proposes to move it from `clojure.spec` to `clojure.core` as a public var named `var->sym` and to refactor the two variants to use it.

Patch: clj-1952-v2.patch
Prescreened by: Alex Miller



 Comments   
Comment by Jozef Wagner [ 01/Dec/16 1:38 PM ]

Added patch

Comment by Alex Miller [ 01/Dec/16 7:58 PM ]

Patch has whitespace warnings that should be removed.

Comment by Jozef Wagner [ 02/Dec/16 2:10 AM ]

Attached updated patch clj-1952-v2.patch that has no trailing whitespaces.





[CLJ-1951] bigint? predicate and generator Created: 08/Jun/16  Updated: 08/Oct/18

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

Type: Feature Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: generator

Approval: Vetted

 Description   

Add bigint? and spec.gen support.

This part is easy:

(defn bigint?
  "Returns true if n is a BigInt"
  {:added "1.9"}
  [n] (instance? clojure.lang.BigInt n))

The generator is the tricky bit. test.check doesn't have a generator for bigints, just large-integer for things in long range. I think we'd want numbers beyond long range in a bigint generator (as that's a likely place where bugs might lie). Making a really high-quality bigint generator (with good growth and shrinking characteristics) is something that needs more thought.

http://clojure.github.io/test.check/clojure.test.check.generators.html#var-large-integer



 Comments   
Comment by Gary Fredericks [ 17/Dec/16 6:17 PM ]

In case I don't get around to making a patch, I think a generator along these lines would be a decent start:

(def gen-bigint 
  (gen/sized 
   (fn [size] 
     (let [large-integer (gen/resize size gen/large-integer)] 
       ;; scaling gives us relatively small vectors, but using  
       ;; the resized large-integer above means the numbers in 
       ;; the small vectors will still be big 
       (gen/scale #(+ 2 (/ % 20))
                  (gen/fmap (fn [xs] (+ (bigint (first xs)) (reduce * (map bigint (rest xs))))) 
                            (gen/not-empty (gen/vector large-integer))))))))
Comment by Gary Fredericks [ 17/Dec/16 6:21 PM ]

If anything seems inadequate about the sizing in the above generator, I should point out that sizing in test.check is rather subtle but the requirements are also not well-defined. I'd be happy to discuss in detail.

Comment by Gary Fredericks [ 15/May/17 4:39 PM ]

As an update, I've put together a marvelous bigint generator that this margin is too small to contain. It might need tweaking but is probably fine for these purposes. It's not on test.check master yet but I expect it will be soon.





[CLJ-1891] New socket server startup proactively loads too much code, slowing boot time Created: 09/Feb/16  Updated: 08/Oct/18

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: server

Attachments: Text File clj-1891.patch    
Patch: Code
Approval: Vetted

 Description   

In the new socket server code, clojure.core.server is proactively loaded (regardless of whether servers are in the config), which will also load clojure.edn and clojure.string.

Approach: Delay loading of this code until the first server config is found. This improves startup time when not using the socket server about 0.05 s.

Patch: clj-1891.patch



 Comments   
Comment by Alexander Yakushev [ 02/Jan/18 9:17 AM ]

Bump. With the introduction of Spec, and considering the fact that clojure.core.server triggers the initialization of Spec, the benefit of solving this issue should now be bigger than 0.05 sec (more like 0.2 sec). See http://clojure-goes-fast.com/blog/clojures-slow-start .





[CLJ-1741] deftype class literals and instances loaded from different classloaders when recompiling namespace Created: 30/May/15  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Stephen Nelson Assignee: Unassigned
Resolution: Unresolved Votes: 2
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-1322] doseq with several bindings causes "ClassFormatError: Invalid Method Code length" Created: 10/Jan/14  Updated: 08/Oct/18

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

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

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


Attachments: Text File doseq-bench.txt     Text File doseq.patch     File script.clj    
Patch: Code
Approval: Incomplete

 Description   

Important Perf Note the new impl is faster for collections that are custom-reducible but not chunked, and is also faster for large numbers of bindings. The original implementation is hand tuned for chunked collections, and wins for larger chunked coll/smaller binding count scenarios, presumably due to the fn call/return tracking overhead of reduce. Details are in the comments.
Screened By
Patch doseq.patch

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

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



 Comments   
Comment by Kevin Downey [ 18/Apr/14 12:20 AM ]

reproduces with jdk 1.8.0 and clojure 1.6

Comment by Nicola Mometto [ 22/Apr/14 5:35 PM ]

A potential fix for this is to make doseq generate intermediate fns like `for` does instead of expanding all the code directly.

Comment by Ghadi Shayban [ 25/Jun/14 8:39 PM ]

Existing doseq handles chunked-traversal internally, deciding the
mechanics of traversal for a seq. In addition to possibly conflating
concerns, this is causing a code explosion blowup when more bindings are
added, approx 240 bytes of bytecode per binding (without modifiers).

This approach redefs doseq later in core.clj, after protocol-based
reduce (and other modern conveniences like destructuring.)

It supports the existing :let, :while, and :when modifiers.

New is a stronger assertion that modifiers cannot come before binding
expressions. (Same semantics as let, i.e. left to right)

valid: [x coll :when (foo x)]
invalid: [:when (foo x) x coll]

This implementation does not suffer from the code explosion problem.
About 25 bytes of bytecode + 1 fn per binding.

Implementing this without destructuring was not a party, luckily reduce
is defined later in core.

Comment by Andy Fingerhut [ 26/Jun/14 12:25 AM ]

For anyone reviewing this patch, note that there are already many tests for correct functionality of doseq in file test/clojure/test_clojure/for.clj. It may not be immediately obvious, but every test for 'for' defined with deftest-both is a test for 'for' and also for 'doseq'.

Regarding the current implementation of doseq: it in't simply that it is too many bytes per binding, it is that the code size doubles with each additional binding. See these results, which measures size of the macroexpanded form rather than byte code size, but those two things should be fairly linearly related to each other here:

(defn formsize [form]
  (count (with-out-str (print (macroexpand form)))))

user=> (formsize '(doseq [x (range 10)] (print x)))
652
user=> (formsize '(doseq [x (range 10) y (range 10)] (print x y)))
1960
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10)] (print x y z)))
4584
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10)] (print x y z w)))
9947
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10) p (range 10)] (print x y z w p)))
20997

Here are results for the same expressions after Ghadi's patch doseq.patch dated June 25 2014:

user=> (formsize '(doseq [x (range 10)] (print x)))
93
user=> (formsize '(doseq [x (range 10) y (range 10)] (print x y)))
170
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10)] (print x y z)))
247
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10)] (print x y z w)))
324
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10) p (range 10)] (print x y z w p)))
401

It would be good to see some performance results with and without this patch, too.

Comment by Stuart Halloway [ 28/Jun/14 2:21 PM ]

In the tests below, the new impl is called "doseq2", vs. the original impl "doseq"

(def hund (into [] (range 100)))
(def ten (into [] (range 10)))
(def arr (int-array 100))
(def s "superduper")

;; big seq, few bindings: doseq2 LOSES
(dotimes [_ 5]
  (time (doseq [a (range 100000000)])))
;; 1.2 sec

(dotimes [_ 5]
  (time (doseq2 [a (range 100000000)])))
;; 1.8 sec

;; small unchunked reducible, few bindings: doseq2 wins
(dotimes [_ 5]
  (time (doseq [a s b s c s])))
;; 0.5 sec

(dotimes [_ 5]
  (time (doseq2 [a s b s c s])))
;; 0.2 sec

(dotimes [_ 5]
  (time (doseq [a arr b arr c arr])))
;; 40 msec

(dotimes [_ 5]
  (time (doseq2 [a arr b arr c arr])))
;; 8 msec

;; small chunked reducible, few bindings: doseq2 LOSES
(dotimes [_ 5]
  (time (doseq [a hund b hund c hund])))
;; 2 msec

(dotimes [_ 5]
  (time (doseq2 [a hund b hund c hund])))
;; 8 msec

;; more bindings: doseq2 wins bigger and bigger
(dotimes [_ 5]
  (time (doseq [a ten b ten c ten d ten ])))
;; 2 msec

(dotimes [_ 5]
  (time (doseq2 [a ten b ten c ten d ten ])))
;; 0.4 msec

(dotimes [_ 5]
  (time (doseq [a ten b ten c ten d ten e ten])))
;; 18 msec

(dotimes [_ 5]
  (time (doseq2 [a ten b ten c ten d ten e ten])))
;; 1 msec
Comment by Ghadi Shayban [ 28/Jun/14 6:23 PM ]

Hmm, I cannot reproduce your results.

I'm not sure whether you are testing with lein, on what platform, what jvm opts.

Can we test using this little harness instead directly against clojure.jar? I've attached a the harness and two runs of results (one w/ default heap, the other 3GB w/ G1GC)

I added a medium and small (range) too.

Anecdotally, I see doseq2 outperform in all cases except the small range. Using criterium shows a wider performance gap favoring doseq2.

I pasted the results side by side for easier viewing.

core/doseq                          doseq2
"Elapsed time: 1610.865146 msecs"   "Elapsed time: 2315.427573 msecs"
"Elapsed time: 2561.079069 msecs"   "Elapsed time: 2232.479584 msecs"
"Elapsed time: 2446.674237 msecs"   "Elapsed time: 2234.556301 msecs"
"Elapsed time: 2443.129809 msecs"   "Elapsed time: 2224.302855 msecs"
"Elapsed time: 2456.406103 msecs"   "Elapsed time: 2210.383112 msecs"

;; med range, few bindings:
core/doseq                          doseq2
"Elapsed time: 28.383197 msecs"     "Elapsed time: 31.676448 msecs"
"Elapsed time: 13.908323 msecs"     "Elapsed time: 11.136818 msecs"
"Elapsed time: 18.956345 msecs"     "Elapsed time: 11.137122 msecs"
"Elapsed time: 12.367901 msecs"     "Elapsed time: 11.049121 msecs"
"Elapsed time: 13.449006 msecs"     "Elapsed time: 11.141385 msecs"

;; small range, few bindings:
core/doseq                          doseq2
"Elapsed time: 0.386334 msecs"      "Elapsed time: 0.372388 msecs"
"Elapsed time: 0.10521 msecs"       "Elapsed time: 0.203328 msecs"
"Elapsed time: 0.083378 msecs"      "Elapsed time: 0.179116 msecs"
"Elapsed time: 0.097281 msecs"      "Elapsed time: 0.150563 msecs"
"Elapsed time: 0.095649 msecs"      "Elapsed time: 0.167609 msecs"

;; small unchunked reducible, few bindings:
core/doseq                          doseq2
"Elapsed time: 2.351466 msecs"      "Elapsed time: 2.749858 msecs"
"Elapsed time: 0.755616 msecs"      "Elapsed time: 0.80578 msecs"
"Elapsed time: 0.664072 msecs"      "Elapsed time: 0.661074 msecs"
"Elapsed time: 0.549186 msecs"      "Elapsed time: 0.712239 msecs"
"Elapsed time: 0.551442 msecs"      "Elapsed time: 0.518207 msecs"

core/doseq                          doseq2
"Elapsed time: 95.237101 msecs"     "Elapsed time: 55.3067 msecs"
"Elapsed time: 41.030972 msecs"     "Elapsed time: 30.817747 msecs"
"Elapsed time: 42.107288 msecs"     "Elapsed time: 19.535747 msecs"
"Elapsed time: 41.088291 msecs"     "Elapsed time: 4.099174 msecs"
"Elapsed time: 41.03616 msecs"      "Elapsed time: 4.084832 msecs"

;; small chunked reducible, few bindings:
core/doseq                          doseq2
"Elapsed time: 31.793603 msecs"     "Elapsed time: 40.082492 msecs"
"Elapsed time: 17.302798 msecs"     "Elapsed time: 28.286991 msecs"
"Elapsed time: 17.212189 msecs"     "Elapsed time: 14.897374 msecs"
"Elapsed time: 17.266534 msecs"     "Elapsed time: 10.248547 msecs"
"Elapsed time: 17.227381 msecs"     "Elapsed time: 10.022326 msecs"

;; more bindings:
core/doseq                          doseq2
"Elapsed time: 4.418727 msecs"      "Elapsed time: 2.685198 msecs"
"Elapsed time: 2.421063 msecs"      "Elapsed time: 2.384134 msecs"
"Elapsed time: 2.210393 msecs"      "Elapsed time: 2.341696 msecs"
"Elapsed time: 2.450744 msecs"      "Elapsed time: 2.339638 msecs"
"Elapsed time: 2.223919 msecs"      "Elapsed time: 2.372942 msecs"

core/doseq                          doseq2
"Elapsed time: 28.869393 msecs"     "Elapsed time: 2.997713 msecs"
"Elapsed time: 22.414038 msecs"     "Elapsed time: 1.807955 msecs"
"Elapsed time: 21.913959 msecs"     "Elapsed time: 1.870567 msecs"
"Elapsed time: 22.357315 msecs"     "Elapsed time: 1.904163 msecs"
"Elapsed time: 21.138915 msecs"     "Elapsed time: 1.694175 msecs"
Comment by Ghadi Shayban [ 28/Jun/14 6:47 PM ]

It's good that the benchmarks contain empty doseq bodies in order to isolate the overhead of traversal. However, that represents 0% of actual real-world code.

At least for the first benchmark (large chunked seq), adding in some tiny amount of work did not change results signifantly. Neither for (map str [a])

(range 10000000) =>  (map str [a])
core/doseq
"Elapsed time: 586.822389 msecs"
"Elapsed time: 563.640203 msecs"
"Elapsed time: 369.922975 msecs"
"Elapsed time: 366.164601 msecs"
"Elapsed time: 373.27327 msecs"
doseq2
"Elapsed time: 419.704021 msecs"
"Elapsed time: 371.065783 msecs"
"Elapsed time: 358.779231 msecs"
"Elapsed time: 363.874448 msecs"
"Elapsed time: 368.059586 msecs"

nor for intrisics like (inc a)

(range 10000000)
core/doseq
"Elapsed time: 317.091849 msecs"
"Elapsed time: 272.360988 msecs"
"Elapsed time: 215.501737 msecs"
"Elapsed time: 206.639181 msecs"
"Elapsed time: 206.883343 msecs"
doseq2
"Elapsed time: 241.475974 msecs"
"Elapsed time: 193.154832 msecs"
"Elapsed time: 198.757873 msecs"
"Elapsed time: 197.803042 msecs"
"Elapsed time: 200.603786 msecs"

I still see reduce-based doseq ahead of the original, except for small seqs

Comment by Ghadi Shayban [ 04/Aug/14 2:55 PM ]

A form like the following will not work with this patch:

(go (doseq [c chs] (>! c :foo)))

as the go macro doesn't traverse fn boundaries. The only such code I know is core.async/mapcat*, a private fn supporting a fn that is marked deprecated.

Comment by Ghadi Shayban [ 07/Aug/14 2:09 PM ]

I see #'clojure.core/run! was just added, which has a similar limitation

Comment by Rich Hickey [ 29/Aug/14 8:19 AM ]

Please consider Ghadi's feedback, esp re: closures.

Comment by Ghadi Shayban [ 22/Sep/14 4:36 PM ]

The current expansion of a doseq [1] under a go form is less than ideal due to the amount of control flow. 14 states in the state machine vs. 7 with loop/recur

[1] Comparison of macroexpansion of (go ... doseq) vs (go ... loop/recur)
https://gist.github.com/ghadishayban/639009900ce1933256a1

Comment by Nicola Mometto [ 25/Mar/15 6:07 PM ]

Related: CLJ-77

Comment by Nicola Mometto [ 03/Sep/15 12:59 PM ]

The general solution to this would be to automatically split methods when too large, using something like https://bitbucket.org/sperber/asm-method-size

Comment by Ghadi Shayban [ 24/Apr/16 12:25 AM ]

Example doseq impl and macroexpansion that does not suffer exponential bytecode growth. It also doesn't use any lambdas, so suitable for core.async.
https://gist.github.com/ghadishayban/fe84eb8db78f23be68e20addf467c4d4
It uses an explicit stack for the seqs/bindings.
It does not handle any chunking or modifiers yet.





[CLJ-2371] [spec] alt with an empty cat breaks explain Created: 06/Jul/18  Updated: 08/Oct/18

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9, Release 1.10
Fix Version/s: Release 1.11

Type: Defect Priority: Major
Reporter: jcr Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Vetted

 Description   
(s/conform (s/alt :empty (s/cat)) [42]) ; => ::s/invalid
(s/explain (s/alt :empty (s/cat)) [42]) ; => prints 'Success!'

Related:
CLJ-2360 - looks like the same issue, but for s/and, s/or
CLJ-2336 - looks like a consequence of this issue
CLJ-2304 - probably related too






[CLJ-2249] Document behavior of clojure.core/get on strings and Java arrays Created: 05/Oct/17  Updated: 08/Oct/18

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

Type: Enhancement Priority: Minor
Reporter: Arne Brasseur Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File 0001-Expand-docstring-of-clojure.core-get.patch     Text File clj-2249-3.patch     Text File CLJ-2249-patch-2.patch    
Patch: Code
Approval: Vetted

 Description   

get's implementation checks in order for

  • ILookup
  • nil
  • Map
  • IPersistentSet
  • String or Java array

The docstring for get currently reads

"Returns the value mapped to key, not-found or nil if key not present."

That this works on maps and associative data can reasonably be inferred if one knows Clojure's data model. That it works on sets, Strings, and arrays is less obvious, and would be helpful to mention.

Patch: clj-2249-3.patch



 Comments   
Comment by Arne Brasseur [ 05/Oct/17 12:49 PM ]

I've tried to stick to the same terse style, happy to take suggestions for phrasing. I also changed the first sentence slightly, because I found it difficult to parse, but that is an unrelated change that I can undo if that's preferred.

Comment by Alex Miller [ 05/Oct/17 1:14 PM ]

I'd leave the first sentence as is.

All of these collection ops are kind of tricky in being able to succinctly state the intent, while also covering special cases (which are often related to Java types). Here I think the intent is to cover lookup in "associative data structures" which covers Clojure maps, records, vectors, Java maps, and other less obvious things like weird ILookup impls.

The non-obvious inclusions for me are: Clojure sets (I haven't reviewed but undoubtedly this is implicitly used in a bunch of special cases), and the Java special cases which are Strings and arrays. For an example of wording, I would point to `count` and `nth`, which are similarly weird.

So maybe a sentence like: "get also works on sets to return contained values, and on strings and arrays for value by index." ?

We'll need to answer these same questions in the spec too btw. I expect the act of spec'ing core fns to drive more of these tricky questions.

Comment by David Liepmann [ 05/Oct/17 2:18 PM ]

>I'd leave the first sentence as is.

Would a rephrase be welcome in its own issue? Right now the referent of "the value mapped to" is ambiguous. I agree it's difficult to parse.

Comment by Alex Miller [ 05/Oct/17 2:50 PM ]

David Liepmann no thanks

Comment by Arne Brasseur [ 09/Oct/17 3:27 AM ]

Appended new patch.

Comment by Alex Miller [ 04/Oct/18 2:12 PM ]

Reworked docstring addition a bit





[CLJ-2202] [spec] coll-of :min-count and :gen-max used together cause collections that are too large to be generated Created: 10/Jul/17  Updated: 08/Oct/18

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

Type: Defect Priority: Minor
Reporter: Sebastian Oberhoff Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

1.9.0-alpha16


Attachments: Text File clj-2202.patch    
Patch: Code
Approval: Vetted

 Description   

This should specify a spec whose generator always returns collections of size 4 or 5 but instead it generates collections of size 4 to 8:

(require '[clojure.spec.alpha :as s] '[clojure.spec.gen.alpha :as gen])
(map count (gen/sample (s/gen (s/coll-of char? :min-count 4 :gen-max 5))))
;; (4 7 8 8 4 7 4 5 5 7)

Cause: The max logic in s/every-impl is: (c/or max-count (max gen-max (c/* 2 (c/or min-count 0)))). If there is a max-count it's used, otherwise the larger of gen-max or 2*min-count is used. In this case, 2*min-count is 8. Seems like we should never generate more than gen-max though, so propose changing this logic to: (c/or max-count gen-max (c/* 2 (c/or min-count 0))).

Patch: clj-2202.patch






[CLJ-2174] [spec] Spec generated exceptions/error messages are a regression in terms of the out-of-the-box experience with Clojure. Created: 01/Jun/17  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Russ Olsen Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: errormsgs, spec
Environment:

All


Approval: Triaged

 Description   

While it is clear that spec has a lot of advantages in terms of a uniform way to specify the shape of behavior, using spec to catch programming and API errors within Clojure itself has led to error messages that more verbose and less clear than what was there in previous versions. For example if I supply a bad name to defn, in version an earlier version of Clojure I got a clear, relatively English language messages back:

Clojure 1.7.0

user=> (defn 44 [x] x)
IllegalArgumentException First argument to defn must be a symbol clojure.core/defn--4156 (core.clj:281)

In the current 1.9 release, the same mistake generates the following:

Clojure 1.9.0-master-SNAPSHOT

user=> (defn 44 [x] x)
CompilerException clojure.lang.ExceptionInfo: Call to clojure.core/defn did not conform to spec:
In: [0] val: 44 fails spec: :clojure.core.specs.alpha/defn-args at: [:args :name] predicate: simple-symbol?
:clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x3c0f93f1 "clojure.spec.alpha$regex_spec_impl$reify__1200@3c0f93f1"]
:clojure.spec.alpha/value (44 [x] x)
:clojure.spec.alpha/args (44 [x] x)
#:clojure.spec.alpha{:problems [{:path [:args :name], :pred clojure.core/simple-symbol?, :val 44, :via [:clojure.core.specs.alpha/defn-args :clojure.core.specs.alpha/defn-
args], :in [0]}], :spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x3c0f93f1 "clojure.spec.alpha$regex_spec_impl$reify__1200@3c0f93f1"], :value (44 [x] x), :args
(44 [x] x)}, compiling:(NO_SOURCE_PATH:1:1)

There is a similar situation with let. Here is the behavior in earlier versions:

user-> (let (a 1) (println a))
IllegalArgumentException let requires a vector for its binding in user:1 clojure.core/let (core.clj:4309)

And in 1.9:

user=> (let (a 1) (println a))

CompilerException clojure.lang.ExceptionInfo: Call to clojure.core/let did not conform to spec:
In: [0] val: (a 1) fails spec: :clojure.core.specs.alpha/bindings at: [:args :bindings] predicate: vector?
:clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x31dc339b "clojure.spec.alpha$regex_spec_impl$reify__1200@31dc339b"]
:clojure.spec.alpha/value ((a 1) (println a))
:clojure.spec.alpha/args ((a 1) (println a))
#:clojure.spec.alpha{:problems [{:path [:args :bindings], :pred clojure.core/vector?, :val (a 1), :via [:clojure.core.specs.alpha/bindings
:clojure.core.specs.alpha/bindings], :in [0]}], :spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x31dc339b
"clojure.spec.alpha$regex_spec_impl$reify__1200@31dc339b"], :value ((a 1) (println a)), :args ((a 1) (println a))}, compiling:(NO_SOURCE_PATH:1:1)

Yes all of the information – and more – is there in the 1.9 version.
But the spec error messages are likely to be incomprehensible to anyone relatively new to Clojure and adds to the cognitive load of even experienced Clojure programmers.



 Comments   
Comment by Russ Olsen [ 01/Jun/17 9:15 AM ]

Typos in that first sentence, should have read 'shape of DATA' not behavior, and 'error messages that ARE more verbose'.

Comment by Alex Miller [ 01/Jun/17 2:19 PM ]

Hey Russ, part of this is actually a bug that crept into alpha17 that is tracked with a patch here: https://dev.clojure.org/jira/browse/CLJ-2171

But also, I would like to overhaul the instrument exception reporting as I think it could be a lot clearer about the signature being invoked and how it is wrong.





[CLJ-2079] [spec] Generator overrides for spec aliases are not respected Created: 08/Dec/16  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Nate Smith Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: generator, spec

Approval: Vetted

 Description   

Generator overrides for spec aliases are not respected.

(require '[clojure.spec :as s])
(require '[clojure.spec.gen :as gen])
(s/def ::original number?)
(s/def ::alias ::original)

(every? double? (gen/sample (s/gen ::alias {::alias gen/double})))
;; => false

Providing a generator override for the original spec works as expected:

(every? double? (gen/sample (s/gen ::alias {::original gen/double})))
;; => true


 Comments   
Comment by Alex Miller [ 08/Dec/16 5:02 PM ]

Probably a missing delay in the alias case - there's another ticket that has the same cause.

Comment by Nate Smith [ 08/Dec/16 6:43 PM ]

Looks like it might be because gensub looks for matching overrides by calling spec-name, which returns the wrong value for spec aliases.

(require '[clojure.spec :as s])
(s/def ::original number?)
(s/def ::alias ::original)
(@#'clojure.spec/spec-name (s/get-spec ::alias))
;; => :user/original
Comment by Charles Despointes [ 20/Jun/17 1:19 PM ]

I've a somewhat similar issue. I think it is related.
I'm trying to do something like :

(require '[clojure.spec.alpha :as s])
(require '[clojure.spec.gen.alpha :as gen])
(s/def ::bar any?)
(s/def ::foo (s/with-gen any? (fn [] (s/gen ::bar))))
(gen/generate (s/gen ::foo {::bar (fn [] (s/gen int?))}))

I'm somewhat expecting it generates me an integer like it would have with a direct aliasing to ::bar in ::foo definition. But it doesn't and keep the with-gen binded generator.
Is that the same issue or is that an expected behaviour or should i fill a new issue ?

Comment by James Gatannah [ 04/Mar/18 12:07 AM ]

I think I'm probably running into this, except that there seems to be some non-determinism involved.

It seems to behave differently, depending on whether I run it from within a deftest or from the REPL. (Running it from the REPL seems to fail every time. Running it inside deftest seems much more reliable).

My really long-winded description is at https://groups.google.com/forum/#!topic/clojure/zPWPmQGm94w

The sample where I tried to document exactly what I'm seeing is at https://gist.github.com/jimrthy/21851c52a8cd6b04a31ed08b1d0a7f04 (when I wrote that, running inside deftest seemed to work every time. That is not the case).

If nothing else, it would be nice to have better error messages that include the name of the spec it's failing to generate. Actually, that would be generally helpful and possibly worth its own ticket. Please let me know if you'd like me to create one.





[CLJ-2003] [spec] Nesting cat inside ? causes unform to return nested result Created: 11/Aug/16  Updated: 08/Oct/18

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

Type: Defect Priority: Critical
Reporter: Sam Estep Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: spec

Attachments: Text File CLJ-2003-corrected.patch     Text File CLJ-2003.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Calling conform and then unform with a spec that consists of some cat nested inside of some ? creates an extra level of nesting in the result:

(require '[clojure.spec :as s])

(let [spec (s/? (s/cat :foo #{:foo}))
      initial [:foo]
      conformed (s/conform spec initial)
      unformed (s/unform spec conformed)]
  [initial conformed unformed])
;;=> [[:foo] {:foo :foo} [(:foo)]]

This behavior does not occur with just ? or cat alone:

(let [spec (s/? #{:foo})]
  (s/unform spec (s/conform spec [:foo])))
;;=> [:foo]

(let [spec (s/cat :foo #{:foo})]
  (s/unform spec (s/conform spec [:foo])))
;;=> (:foo)

Patch: CLJ-2003-corrected.patch



 Comments   
Comment by Phil Brown [ 14/Aug/16 9:55 PM ]

I came across another case of extra nesting, when repeating one or more sequences with an optional element at the beginning or end, where that element's predicate also matches the element at the other end:

user=> (s/conform (s/+ (s/cat :k any? :v (s/? any?))) [:a 1 :b 2])
[{:k :a, :v 1} [{:k :b, :v 2}]]

where I expected

[{:k :a, :v 1} {:k :b, :v 2}]

The following give expected results:

user=> (s/conform (s/+ (s/cat :k any? :v (s/? any?))) [:a 1 :b])
[{:k :a, :v 1} {:k :b}]
user=> (s/conform (s/+ (s/cat :k keyword? :v (s/? int?))) [:a 1 :b 2])
[{:k :a, :v 1} {:k :b, :v 2}]
user=> (s/conform (s/* (s/cat :k any? :v (s/? any?))) [:a 1 :b 2])
[{:k :a, :v 1} {:k :b, :v 2}]
Comment by Alex Miller [ 01/Sep/16 11:06 AM ]

Phil, I think your example is a different issue and you should file a new jira for that.

Comment by Alex Miller [ 01/Sep/16 3:05 PM ]

Well, maybe I take that back, they may be related.

Comment by Brandon Bloom [ 08/Nov/16 6:10 PM ]

I just ran in to this trying to make sense of some defn forms. Here's an example:

user=> (s/unform :clojure.core.specs/defn-args (s/conform :clojure.core.specs/defn-args '(f [& xs])))
(f ((& xs)))

Comment by Tyler Tallman [ 09/Aug/17 9:41 PM ]

This seems to be all that is needed.

Comment by Francis Avila [ 28/Aug/17 9:24 PM ]

The problem is actually more universal than (? (cat ...)). s/unform of a s/? with any regex child op will introduce an extra level of nesting. When the child is a regex, we are consuming the same "level" of sequence so unform should not introduce an extra level. However in other cases (non-regex ops), we should still possibly produce a nested collection.

The previous patch was too aggressive: it unwrapped all sub-unforms of s/?. This patch CLJ-2003-corrected.patch only unwraps when the sub-op is a regex.

Unfortunately it is impossible to distinguish between a desired-but-optional nil and a non-match from s/?. Specifically, the following tests now hold:

(testing "s/? matching nil"
  (is (nil? (s/conform (s/? nil?) [nil])))
  (is (nil? (s/conform (s/? nil?) [])))
  (is (nil? (s/conform (s/? nil?) nil)))
  (is (= (s/unform (s/? nil?) nil) [])))

(I did not add these tests to the patch because I was unsure if they should be part of the contract of unform. However, they are pretty big gotchas.)

I also added tests for every possible subop of s/?, except ::s/accept, which I could not think of a test case for. (I'm not sure ::s/accept is actually reachable inside s/op-unform?)

Comment by Alex Miller [ 29/Aug/17 7:33 AM ]

Thanks for working on this - I will take a look when I get a chance.

Comment by Francis Avila [ 29/Aug/17 9:20 AM ]

I had to amend my patch slightly (same name): one of the test cases wasn't testing the correct thing.

Comment by Francis Avila [ 31/Jul/18 1:12 PM ]

Patch was no longer applying cleanly to master because of tests added by other commits. Patch rebased to master





[CLJ-1973] generate-proxy produces unpredictable method order in generated classes Created: 01/Jul/16  Updated: 08/Oct/18

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

Type: Enhancement Priority: Minor
Reporter: James Carnegie Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: aot, compiler
Environment:

OSX, OpenJDK 8


Attachments: Text File CLJ-1973-v1.patch     Text File CLJ-1973-v2.patch     Text File CLJ-1973-v3.patch     Text File CLJ-1973-v4.patch     Text File CLJ-1973-v5.patch    
Patch: Code
Approval: Vetted

 Description   

Using core/proxy to generate proxies for Java classes produces unpredictable method ordering in the generated class files.
This is a problem for repeatable builds (when doing AOT).

Specifically, I'm running Clojure inside Docker, and I'd like my application image layer to be as small as those produced by Java developers (using Meta-inf classpaths and a lib directory). Anyway, to get this working properly so that all dependencies (including those compiled as part of AOT) are on a separate layer, I need the output of compiling my applications' dependencies' proxies to be the same each time I run the build. This reduces build time, image push time, image pull time and container scheduling time.

Example code that exhibits the problem (you'll need to run it a few times to see the issue).

https://github.com/kipz/predictable-proxies

Cause: I've tracked it down to the use of an unsorted map here:

https://github.com/clojure/clojure/blob/master/src/clj/clojure/core_proxy.clj#L186

Approach: Use a sorted map, sorted by hash of the key (which is a vector of method name, seq of param types, and return type).

Patch: CLJ-1973-v5.patch



 Comments   
Comment by James Carnegie [ 01/Jul/16 4:19 PM ]

Patch that uses a sorted-map

Comment by Alex Miller [ 04/Jul/16 9:44 AM ]

I think you can follow the advice at http://clojure.org/guides/comparators to write a simpler comparator for this patch.

Comment by James Carnegie [ 04/Jul/16 11:24 AM ]

Simpler comparator as requested.

Comment by Alex Miller [ 04/Jul/16 12:28 PM ]

I think you lost the sorted set.

Comment by James Carnegie [ 04/Jul/16 1:06 PM ]

Copy paste between branches error. Tested this time.

Comment by James Carnegie [ 04/Jul/16 1:14 PM ]

Now with more consistent formatting of 'let'

Comment by Alex Miller [ 04/Jul/16 2:27 PM ]

While this is probably fine, it might be better to use the hash (Clojure) function rather than .hashCode (Java) function. The map itself is hashed based on the hash so that seems more appropriate.

Comment by James Carnegie [ 04/Jul/16 3:15 PM ]

As requested, using Clojure 'hash' instead.

Thanks Alex - learned about boolean comparators too!

Comment by Alex Miller [ 04/Jul/16 3:52 PM ]

Note that this ordering may still change across Clojure or JVM versions as there is no guarantee of hashing across those. Pre-screening for now.





[CLJ-1965] [spec] clojure.spec/def should support an optional doc-string Created: 19/Jun/16  Updated: 08/Oct/18

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

Type: Feature Priority: Critical
Reporter: Alexander Kiel Assignee: Unassigned
Resolution: Unresolved Votes: 63
Labels: spec

Approval: Vetted

 Description   

Like clojure.core/def clojure.spec/def should support an optional doc string because one usually likes to describe specs in more detail as one could through keyword naming.



 Comments   
Comment by Moritz Heidkamp [ 03/Nov/16 4:23 PM ]

Building on this idea, I suggest to add first-class metadata support to registered specs and implement doc strings in terms of that (i.e. the same way as with vars).

Comment by Josh Brandoff [ 01/Apr/17 5:02 PM ]

Hi! Was just discussing the potential for a feature like this with a colleague. What's the current status? Was thinking of potentially working on it but wanted to get feedback and guidance from the community first.

Comment by Alex Miller [ 03/Apr/17 9:32 AM ]

We don't have a recommended approach to this yet so not looking for a patch at this time.

Comment by Felix Andrews [ 19/Jun/18 7:30 PM ]

If you're prioritising this, please also remember the need for source (file + line) metadata on specs. This would enable us to use specs in auto generated documentation which is really nice, especially when you can hyperlink to nested specs. See CLJ-2037 https://dev.clojure.org/jira/browse/CLJ-2037





[CLJ-1941] [spec] Instrumentation of fns with primitive type hints fails Created: 01/Jun/16  Updated: 08/Oct/18

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

Type: Defect Priority: Minor
Reporter: Kenny Williams Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: spec
Environment:

Ubuntu 15.10
Using boot 2.6.0 on openjdk version "1.8.0_91"


Approval: Vetted

 Description   
(require '[clojure.spec :as s] '[clojure.spec.test :as st])
(defn foo [^double val] val)
(s/fdef foo :args (s/cat :val double?))
(st/instrument `foo)
(foo 5.2)

user=> (foo 5.2)
ClassCastException clojure.spec.test$spec_checking_fn$fn__13069 cannot be cast to clojure.lang.IFn$DO
       	user/eval6 (NO_SOURCE_FILE:5)
       	user/eval6 (NO_SOURCE_FILE:5)
       	clojure.lang.Compiler.eval (Compiler.java:6951)
       	clojure.lang.Compiler.eval (Compiler.java:6914)
       	clojure.core/eval (core.clj:3187)
       	clojure.core/eval (core.clj:3183)
       	clojure.main/repl/read-eval-print--9704/fn--9707 (main.clj:241)
       	clojure.main/repl/read-eval-print--9704 (main.clj:241)
       	clojure.main/repl/fn--9713 (main.clj:259)
       	clojure.main/repl (main.clj:259)
       	clojure.main/repl-opt (main.clj:323)
       	clojure.main/main (main.clj:422)

Cause: spec replaces var values with instrumented functions that will not work with primitive function interfaces

Approach: Take primitive interfaces into account and make them work, or document/fail that instrumentation will not work with these.



 Comments   
Comment by Kevin Downey [ 02/Jun/16 1:41 AM ]

spec replaces var values with instrumented functions, which works for the default linking case, var deref cast to ifn, invoke, but in the other cases (primitive functions, direct linking, others?) this won't work

Comment by Kenny Williams [ 02/Jun/16 3:39 PM ]

Hmm. Well this should be at least be documented. So, spec cannot be used on functions with a type hinted arg?

Comment by Sean Corfield [ 02/Jun/16 4:16 PM ]

Spec cannot be used on functions with primitive typed hinted arguments or returns – non-primitive type hints seem to be fine.

But documentation isn't enough here: instrumenting a namespace and then discovering it broke a function (that happened to have a primitive type hint) isn't acceptable. If the instrumentation isn't going to work, the function should be skipped (and a warning produced, hopefully).

Comment by Kevin Downey [ 02/Jun/16 8:10 PM ]

yeah, I was giving the root cause of the issue, not excusing the issue.

Understanding the root cause predicts other places where there will be issues: where ever some non-default function linking strategy is used.

One such place is direct linked functions, but I suspect for direct linked functions, Clojure/Core will just say you should only instrument code for testing, and you should only turn on direct liking for production.

Another case, which I am sort of surprised we haven't heard more about yet is protocol functions.

Comment by Sean Corfield [ 02/Jun/16 8:35 PM ]

Your comment about direct linking made me wonder about the validity of spec'ing and instrumenting clojure.core functions. The examples show clojure.core/symbol, but Clojure's core library is shipped as direct linked, as of 1.8.0 isn't it?

Comment by Kevin Downey [ 03/Jun/16 3:14 AM ]

what alters the calling convention isn't the function being compiled with direct linking on, but a caller of that function being compiled with direct linking on.

This code will throw a non-conforming error for the bogus symbol spec with direct linking off, and return the symbol foo with direct linking on

(require '[clojure.spec :as s])

(s/fdef symbol
  :args string?
  :ret symbol?)

(defn foo
  []
  (symbol 'foo))

(s/instrument-all)

(foo)
Comment by Kevin Downey [ 03/Jun/16 3:26 AM ]

This code returns true because m is a protocol function, if you replace it with a regular function it throws a non-conforming error

(require '[clojure.spec :as s])

(defprotocol P
  (m [_]))

(deftype T []
  P
  (m [_] true))

(s/fdef m
  :args (s/cat :p (constantly false))
  :ret string?)

(defn foo
  []
  (m (T.)))

(s/instrument-all)

(foo)
Comment by Alex Miller [ 13/Jun/16 3:53 PM ]

@Sean instrumenting core functions will work for calls from your code into core (which are presumably not direct-linked), but will not affect calls from one core function to another as they are direct-linked and do not go through the var. One thing we've considered for a long while is building a dev version of core that would not be direct-linked and could potentially turn on instrumentation or other helpful dev-time tooling.

Comment by Sean Corfield [ 14/Jun/16 5:48 PM ]

Thanks for that answer @alexmiller – We have dev set to non-direct-linking but QA / production set to direct linking, so I'm only concerned about possible issues in dev with (s/instrumental-all) and wanting to be sure "code won't break". If instrumentation won't affect existing (direct-linked) calls within core, that's good enough for me. I am concerned about primitive hinting and protocols (and whatever crawls out of the woodwork here) since you don't want to be forced to read the source of every library you include, just to see whether (s/instrument-all) is safe or whether it will bite you in some weird way while you're developing.





[CLJ-1888] AReference#meta() is synchronized Created: 26/Jan/16  Updated: 08/Oct/18

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

Type: Enhancement Priority: Major
Reporter: Roger Kapsi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: PNG File aref-meta-after.png     PNG File aref-meta.png     Text File clj-1888-2.patch     Text File clj-1888.patch    
Patch: Code
Approval: Vetted

 Description   

We use Clojure for a "rules engine". Each function represents a rule and metadata describes the rule and provides some static configuration for the rule itself. The system is immutable and concurrent.

If two or more Threads invoke the same Var concurrently they end up blocking each other because AReference#meta() is synchronized (see attached screenshot, the red dots).

(defn 
  ^{:rule {:remote-address "127.0.0.1"}}
  example
  [request]
  (let [rule (:rule (meta #'example))]
    (= (:remote-address rule) (:remote-address request))))

Approach: Replace synchronized block with a rwlock for greater read concurrency. This approach removes meta read contention (see real world example in comments). However, it comes with the downsides of:

  • extra field for every AReference (all namespaces, vars, atoms, refs, and agents)
  • adds construction of lock into construction of AReference (affects perf and startup time)

Patch: clj-1888-2.patch replaces synchronized with a rwlock for greater read concurrency

Alternatives:

  • Use volatile for _meta and synchronized for alter/reset. Allow read of _meta just under the volatile - would this be safe enough?
  • Extend AReference from ReentrantReadWriteLock instead of holding one - this is pretty weird but would have a different (potentially better) footprint for memory/construction.


 Comments   
Comment by Alex Miller [ 26/Jan/16 10:19 PM ]

A volatile is not sufficient in alterMeta as you need to read/update/write atomically.

You could however use a ReadWriteLock instead of synchronized. I've attached a patch that does this - if you have a reproducible case I'd be interested to see how it affects what you see in the profiler.

There are potential issues that would need to be looked at - this will increase memory per reference (the lock instance) and slow down construction (lock construction) at the benefit of more concurrent reads.

Comment by Roger Kapsi [ 27/Jan/16 8:34 AM ]

Hey Alex,

I do have a reproducible case. The blocking has certainly disappeared after applying your patch (see attached picture). The remaining blocking code on these "WorkerThreads" is sun.nio.ch.SelectorImpl.select(long) (i.e. not clojure related).

You can repro it yourself by executing something like the code below concurrently in an infinite loop.

(defn 
  ^{:rule {:remote-address "127.0.0.1"}}
  example
  [request]
  (let [rule (:rule (meta #'example))]
    (= (:remote-address rule) (:remote-address request))))

Suggestions for the patch: Make the meta lock a final field and maybe pull the read/write locks into local variables to avoid the double methods calls.

alterMeta(...)
  Lock w = _metaLock.writeLock();
  w.lock();
  try {
    // ...
  } finally {
    w.unlock();
  }
}
Comment by Alex Miller [ 16/Mar/16 3:02 PM ]

Marking pre-screened,





[CLJ-1872] empty? is broken for transient collections Created: 26/Dec/15  Updated: 08/Oct/18

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

Type: Defect Priority: Critical
Reporter: Leonid Bogdanov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Approval: Vetted

 Description   

Couldn't find whether it was brought up earlier, but it seems that empty? predicate is broken for transient collections

user=> (empty? (transient []))
IllegalArgumentException Don't know how to create ISeq from: clojure.lang.PersistentVector$TransientVector  clojure.lang.RT.seqFrom (RT.java:528)

user=> (empty? (transient ()))
ClassCastException clojure.lang.PersistentList$EmptyList cannot be cast to clojure.lang.IEditableCollection  clojure.core/transient (core.clj:3209)

user=> (empty? (transient {}))
IllegalArgumentException Don't know how to create ISeq from: clojure.lang.PersistentArrayMap$TransientArrayMap  clojure.lang.RT.seqFrom (RT.java:528)

user=> (empty? (transient #{}))
IllegalArgumentException Don't know how to create ISeq from: clojure.lang.PersistentHashSet$TransientHashSet  clojure.lang.RT.seqFrom (RT.java:528)

The workaround is to use (zero? (count (transient ...))) check instead.



 Comments   
Comment by Alex Miller [ 26/Dec/15 9:58 PM ]

Probably similar to CLJ-700.





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

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

Type: Defect Priority: Trivial
Reporter: W. David Jarvis Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, ft
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: Vetted

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

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: ft, performance

Attachments: Text File clj-1730-2.patch     Text File refer-perf.patch     PNG File Screenshot from 2018-08-01 23-23-41.png    
Patch: Code
Approval: Vetted

 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: clj-1730-2.patch

Screening Notes

Patch appears correct but we should consider:

  • non-idiomatic use of if instead of when makes branches hard to read
  • non-idiomatic indentation of if (both branches on one line) hard to read
  • I don't think not found should be an IllegalAccessError, but the original code did this already, so ...
  • the optimistic concurrency loop around the swap will never give up, is this ok?


 Comments   
Comment by Alex Miller [ 07/Sep/17 10:52 AM ]

Patch updated to address first screening comment. I didn't actually find any case of the second one? Give me a pointer.

Comment by Ghadi Shayban [ 01/Aug/18 10:30 PM ]

I suspect that this will not matter much for peak performance. Given the attached picture of clojure.instant loading, this patch may shave a bit off startup time.

(picture made with Bytestacks, profile recorded a JDK11 debug build from today)





[CLJ-1550] Classes generated by deftype and defrecord don't play nice with .getPackage Created: 07/Oct/14  Updated: 08/Oct/18

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

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 15
Labels: classloader, deftype

Attachments: Text File 0001-CLJ-1550-define-package-for-class-in-DynamicClassLoa.patch     Text File CLJ-1550-v2.patch     Text File clj-1550-v3.patch     Text File clj-1550-v4.patch    
Patch: Code and Test
Approval: Incomplete

 Description   

Classes generated loaded by DynamicClassLoader return nil for .getPackage. Tools like CIDER and vim-fireplace are relying on this information to implement things like completion hints.

(.getPackage String)
;; => #<Package package java.lang, Java Platform API Specification, version 1.7>
(deftype T [])
(.getPackage T)
;; => nil

Proposed: During DynamicClassLoader.defineClass(), invoke definePackage() on the class being defined (similar to what URLClassLoader does).

Patch: clj-1550-v4.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 07/Oct/14 8:54 AM ]

According to http://docs.oracle.com/javase/7/docs/api/java/lang/Class.html#getPackage() this method returns the package information found by the class loader or null if there is none. Its not clear to me that the current behavior is wrong per the spec. I would need to experiment more to see if this is unusual or not.

Comment by Bozhidar Batsov [ 07/Oct/14 9:05 AM ]

A bit of background for the issue. I'm no expert on the topic, but being able to procure all the class information except its package definitely looks strange to me.

Comment by Kevin Downey [ 07/Oct/14 11:46 AM ]

if you AOT compile(generate a class file on disk for a deftype), getPackage works fine, which suggests to me it is a jvm issue

Comment by Kevin Downey [ 07/Oct/14 11:49 AM ]

actually, it must just be that dynamicclassloader doesn't define a package for classes it loads

Comment by Alex Miller [ 07/Oct/14 12:13 PM ]

Yep, I believe that's correct.

Comment by Stuart Halloway [ 21/Jul/15 8:01 AM ]

There is no problem statement here. What is package information needed for?

Comment by Bozhidar Batsov [ 21/Jul/15 8:05 AM ]

I've linked the problem above. Basically tools like CIDER and vim-fireplace are relying on this information to implement things like completion hints.
This might not be problem when running your apps, but it's definitely a problem when inspecting their state...

Comment by Michael Blume [ 22/Jul/15 12:32 PM ]

s/Packate/Package

Comment by Alex Miller [ 14/Nov/16 12:10 PM ]

Refreshed patch to apply to current master, attribution retained, no semantic changes. Marked prescreened.

Comment by Ghadi Shayban [ 26/Jun/18 10:55 PM ]

We should consider interactions with the JDK9 module system here

Comment by Alex Miller [ 26/Jun/18 11:28 PM ]

off the top of my head, I don't see how this would do anything negative wrt the JDK9 module system

Comment by Alex Miller [ 29/Jun/18 11:00 AM ]

v4 patch rebases to master and retains attribution, no semantic changes.

Comment by Bozhidar Batsov [ 29/Jun/18 4:18 PM ]

Alex, is this going to be included in Clojure 1.10?

Comment by Alex Miller [ 29/Jun/18 5:31 PM ]

That's the current path...

Comment by Ghadi Shayban [ 04/Oct/18 9:51 PM ]

This is a bad patch that slipped through. The motivation linked above shows an nREPL excerpt that attempts to symbolize the package, basically:

(symbol (.getName (.getPackage TheClass)))
. The original code can derive the package name in the same way the JVM grabs it, from the binary name. (Take the prefix up until the last period.)

The fix also introduces a deprecated call, and more work during classloading (which is critical path).

As an aside, JDK9+ automatically defines a package upon class definition, derived from the binary name:

## JDK 8
➜  clojure git:(f5cfd24d) ✗ docker run --rm -v $HOME/.m2:/m2 openjdk:8-slim java -jar /m2/repository/org/clojure/clojure/1.8.0/clojure-1.8.0.jar -e '(deftype T [])' -e '(or (.getPackage T) :nada)'
user.T
:nada

## JDK 9
➜  clojure git:(f5cfd24d) ✗ docker run --rm -v $HOME/.m2:/m2 openjdk:9-slim java -jar /m2/repository/org/clojure/clojure/1.8.0/clojure-1.8.0.jar -e '(deftype T [])' -e '(or (.getPackage T) :nada)'
user.T
#object[java.lang.Package 0x10993713 "package user"]

## JDK 11
➜  clojure git:(f5cfd24d) ✗ docker run --rm -v $HOME/.m2:/m2 openjdk:11-slim java -jar /m2/repository/org/clojure/clojure/1.8.0/clojure-1.8.0.jar -e '(deftype T [])' -e '(or (.getPackage T) :nada)'
user.T
#object[java.lang.Package 0x6f3c660a "package user"]
➜  clojure git:(f5cfd24d) ✗

I think this should get reverted, and let user space calculate the package from the binary name.

Comment by Bozhidar Batsov [ 04/Oct/18 11:19 PM ]

> The original code can derive the package name in the same way the JVM grabs it, from the binary name. (Take the prefix up until the last period.)

I don't completely understand the workaround you're proposing. Can you elaborate on this?

> I think this should get reverted, and let user space calculate the package from the binary name.

If that's working on newer JDKs with no changes needed in the user space and in Clojure, that's fine by me. By now I had lost faith that was going to be fixed in Clojure anyways.

Comment by Ghadi Shayban [ 05/Oct/18 10:45 AM ]

https://github.com/clojure-emacs/orchard/blob/db6d8a7be853b52c9986c5af8d009639644bb390/src/orchard/java.clj#L157-L159

Took me a while to track it down, but instead of asking for the package name then grabbing the symbol, do this:

(defn package
  [^Class kls]
  (let [kls (.getName kls)
	idx (clojure.string/last-index-of kls \.)]
    (when (pos? idx)
      (subs kls 0 idx))))
Comment by Alex Miller [ 05/Oct/18 10:58 PM ]

Patch reverted





[CLJ-1406] Libs are blindly added into loaded-libs even if an error occurs during loading Created: 17/Apr/14  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Shogo Ohta Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-modify-clojure.core-load-lib-so-that-it-removes-the-.patch     Text File CLJ-1406.patch    
Patch: Code
Approval: Incomplete

 Description   

Suppose you have a lib that causes some errors during loading, like the following:

(ns broken-lib)

(} ; this line will cause a reader error

And then, if you require the lib, it would be added into loaded-libs in spite of the reader error, which makes require succeed silently after that.

user=> (contains? (loaded-libs) 'broken-lib)
false
user=> (require 'broken-lib)
CompilerException java.lang.RuntimeException: Unmatched delimiter: }, compiling:(broken_lib.clj:3:3) 
user=> (contains? (loaded-libs) 'broken-lib)
true
user=> (require 'broken-lib)
nil
user=>

Things get worse if you have another namespace that requires a broken lib (say here broken-lib.core):

(ns broken-lib.core
  (:require [broken-lib :as lib]))

Although you'll get the actual error the first time you load the depending namespace, after that you'll find a wrong compiler exception thrown every time you try to reload it. The situation will last even after you actually do fix the code causing the original error.

user=> (require 'broken-lib.core)

CompilerException java.lang.RuntimeException: Unmatched delimiter: }, compiling:(broken_lib.clj:3:3) 

user=> (require 'broken-lib.core :reload)
CompilerException java.lang.Exception: namespace 'broken-lib' not found, compiling:(broken_lib/core.clj:1:1) 

user=> (require 'broken-lib.core :reload) ;; reload after fix the bug in broken-lib

CompilerException java.lang.Exception: namespace 'broken-lib' not found, compiling:(broken_lib/core.clj:1:1) 
user=>

Cause:
The patch for CLJ-1116 made the ns macro blindly add the lib being defined into loaded-libs even if an error occurs during loading.

Approach:
Modify clojure.core/load-lib so that it removes the lib from loaded-libs on error.



 Comments   
Comment by Alex Miller [ 17/Apr/14 9:07 AM ]

This patch seems somewhat removed from the cause - is there some way to instead prevent the lib from being added to loaded-libs in the first place?

Comment by Shogo Ohta [ 17/Apr/14 9:21 AM ]

To do so, I think we need to revert CLJ-1116.

Comment by Alex Miller [ 22/Sep/16 1:41 PM ]

I don't think this solution is good as is so moving to Incomplete.

Comment by Shogo Ohta [ 15/Oct/16 1:34 AM ]

What kind of solution are you expecting for this problem?

To prevent the lib from being added to loaded-libs in the first place, I think ns macro needs to know where it is used from. It should immediately add the ns when used in the REPL for CLJ-1116, while it should defer adding the ns until completing loading whole the file without errors when used within a file.

Comment by Ghadi Shayban [ 18/Mar/18 2:05 PM ]

Attached a similar approach as Shogo Ohta's original. Given the way ns now works, I see this approach as tenable.

To support dynamic ns creation at the REPL, CLJ-1116 wrote to loaded-libs inside the ns macro, and at the same time made some logic in load-lib and load-libs unnecessary. This set of patches undoes the side-effect when loading fails, and cleans up some dead conditionals.

New behavior: Given ns A which requires #{B, C}, if B loads but C doesn't, only B will be written to loaded-libs, and A and C will be undone. This improves the experience with load and fixes all of the annoying behavior in the ticket description.

NB: the 'require' patch entails:
If a lib is specified in an ns :require or :use, loading that lib the first time must result in an ns that corresponds exactly to the lib name. (There is not and has never been this restriction when calling `load` directly, but only via `ns`) I was surprised to find in the test suite an accidental lib with underscores where the loaded code results in a namespace with a dash. (Perhaps c.c.specs should specify a tighter spec for libs than 'simple-symbol?')

The commits are separated, with accompanying short explanatory comments.

Comment by Alex Miller [ 26/Jun/18 11:52 AM ]

Note - CLJ-2026 may be made irrelevant by any fix to this broader problem and could then be pulled out.





[CLJ-701] Primitive return type of loop and try is lost Created: 03/Jan/11  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Chouser Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: None
Environment:

Clojure commit 9052ca1854b7b6202dba21fe2a45183a4534c501, version 1.3.0-master-SNAPSHOT


Attachments: Text File 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch     Text File 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch     Text File 0002-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch     File hoistedmethod-pass-1.diff     File hoistedmethod-pass-2.diff     File hoistedmethod-pass-3.diff     File hoistedmethod-pass-4.diff     File hoistedmethod-pass-5.diff     File hoistedmethod-pass-6.diff     File hoistedmethod-pass-7.diff    
Patch: Code and Test
Approval: Incomplete

 Description   
(set! *warn-on-reflection* true)
(fn [] (loop [b 0] (recur (loop [a 1] a))))

Generates the following warnings:

recur arg for primitive local: b is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: b

This is interesting for several reasons. For one, if the arg to recur is a let form, there is no warning:

(fn [] (loop [b 0] (recur (let [a 1] a))))

Also, the compiler appears to understand the return type of loop forms just fine:

(use '[clojure.contrib.repl-utils :only [expression-info]])
(expression-info '(loop [a 1] a))
;=> {:class long, :primitive? true}

The problem can of course be worked around using an explicit cast on the loop form:

(fn [] (loop [b 0] (recur (long (loop [a 1] a)))))

Reported by leafw in IRC: http://clojure-log.n01se.net/date/2011-01-03.html#10:31

See Also: CLJ-1422

Patch: 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch



 Comments   
Comment by a_strange_guy [ 03/Jan/11 4:36 PM ]

The problem is that a 'loop form gets converted into an anonymous fn that gets called immediately, when the loop is in a expression context (eg. its return value is needed, but not as the return value of a method/fn).

so

(fn [] (loop [b 0] (recur (loop [a 1] a))))

gets converted into

(fn [] (loop [b 0] (recur ((fn [] (loop [a 1] a))))))

see the code in the compiler:
http://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L5572

this conversion already bites you if you have mutable fields in a deftype and want to 'set! them in a loop

http://dev.clojure.org/jira/browse/CLJ-274

Comment by Christophe Grand [ 23/Nov/12 2:28 AM ]

loops in expression context are lifted into fns because else Hotspot doesn't optimize them.
This causes several problems:

  • type inference doesn't propagate outside of the loop[1]
  • the return value is never a primitive
  • mutable fields are inaccessible
  • surprise allocation of one closure objects each time the loop is entered.

Adressing all those problems isn't easy.
One can compute the type of the loop and emit a type hint but it works only with reference types. To make it works with primitive, primitie fns aren't enough since they return only long/double: you have to add explicit casts.
So solving the first two points can be done in a rather lccal way.
The two other points require more impacting changes, the goal would be to emit a method rather than a fn. So it means at the very least changing ObjExpr and adding a new subclassof ObjMethod.

[1] beware of CLJ-1111 when testing.

Comment by Alex Miller [ 21/Oct/13 10:28 PM ]

I don't think this is going to make it into 1.6, so removing the 1.6 tag.

Comment by Kevin Downey [ 21/Jul/14 7:14 PM ]

an immediate solution to this might be to hoist loops out in to distinct non-ifn types generated by the compiler with an invoke method that is typed to return the getJavaClass() type of the expression, that would give us the simplifying benefits of hoisting the code out and free use from the Object semantics of ifn

Comment by Kevin Downey [ 22/Jul/14 8:39 PM ]

I have attached a 3 part patch as hoistedmethod-pass-1.diff

3ed6fed8 adds a new ObjMethod type to represent expressions hoisted out in to their own methods on the enclosing class

9c39cac1 uses HoistedMethod to compile loops not in the return context

901e4505 hoists out try expressions and makes it possible for try to return a primitive expression (this might belong on http://dev.clojure.org/jira/browse/CLJ-1422)

Comment by Kevin Downey [ 22/Jul/14 8:54 PM ]

with hoistedmethod-pass-1.diff the example code generates bytecode like this

user=> (println (no.disassemble/disassemble (fn [] (loop [b 0] (recur (loop [a 1] a))))))
// Compiled from form-init1272682692522767658.clj (version 1.5 : 49.0, super bit)
public final class user$eval1675$fn__1676 extends clojure.lang.AFunction {
  
  // Field descriptor #7 Ljava/lang/Object;
  public static final java.lang.Object const__0;
  
  // Field descriptor #7 Ljava/lang/Object;
  public static final java.lang.Object const__1;
  
  // Method descriptor #10 ()V
  // Stack: 2, Locals: 0
  public static {};
     0  lconst_0
     1  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16]
     4  putstatic user$eval1675$fn__1676.const__0 : java.lang.Object [18]
     7  lconst_1
     8  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16]
    11  putstatic user$eval1675$fn__1676.const__1 : java.lang.Object [20]
    14  return
      Line numbers:
        [pc: 0, line: 1]

 // Method descriptor #10 ()V
  // Stack: 1, Locals: 1
  public user$eval1675$fn__1676();
    0  aload_0 [this]
    1  invokespecial clojure.lang.AFunction() [23]
    4  return
      Line numbers:
        [pc: 0, line: 1]
  
  // Method descriptor #25 ()Ljava/lang/Object;
  // Stack: 3, Locals: 3
  public java.lang.Object invoke();
     0  lconst_0
     1  lstore_1 [b]
     2  aload_0 [this]
     3  lload_1 [b]
     4  invokevirtual user$eval1675$fn__1676.__hoisted1677(long) : long [29]
     7  lstore_1 [b]
     8  goto 2
    11  areturn
      Line numbers:
        [pc: 0, line: 1]
      Local variable table:
        [pc: 2, pc: 11] local: b index: 1 type: long
        [pc: 0, pc: 11] local: this index: 0 type: java.lang.Object

 // Method descriptor #27 (J)J
  // Stack: 2, Locals: 5
  public long __hoisted1677(long b);
    0  lconst_1
    1  lstore_3 [a]
    2  lload_3
    3  lreturn
      Line numbers:
        [pc: 0, line: 1]
      Local variable table:
        [pc: 2, pc: 3] local: a index: 3 type: long
        [pc: 0, pc: 3] local: this index: 0 type: java.lang.Object
        [pc: 0, pc: 3] local: b index: 1 type: java.lang.Object

}
nil
user=> 
  

the body of the method __hoisted1677 is the inner loop

for reference the part of the bytecode from the same function compiled with 1.6.0 is pasted here https://gist.github.com/hiredman/f178a690718bde773ba0 the inner loop body is missing because it is implemented as its own IFn class that is instantiated and immediately executed. it closes over a boxed version of the numbers and returns an boxed version

Comment by Kevin Downey [ 23/Jul/14 12:43 AM ]

hoistedmethod-pass-2.diff replaces 901e4505 with f0a405e3 which fixes the implementation of MaybePrimitiveExpr for TryExpr

with hoistedmethod-pass-2.diff the largest clojure project I have quick access to (53kloc) compiles and all the tests pass

Comment by Alex Miller [ 23/Jul/14 12:03 PM ]

Thanks for the work on this!

Comment by Kevin Downey [ 23/Jul/14 2:05 PM ]

I have been working through running the tests for all the contribs projects with hoistedmethod-pass-2.diff, there are some bytecode verification errors compiling data.json and other errors elsewhere, so there is still work to do

Comment by Kevin Downey [ 25/Jul/14 7:08 PM ]

hoistedmethod-pass-3.diff

49782161 * add HoistedMethod to the compiler for hoisting expresssions out well typed methods
e60e6907 * hoist out loops if required
547ba069 * make TryExpr MaybePrimitive and hoist tries out as required

all contribs whose tests pass with master pass with this patch.

the change from hoistedmethod-pass-2.diff in this patch is the addition of some bookkeeping for arguments that take up more than one slot

Comment by Nicola Mometto [ 26/Jul/14 1:37 AM ]

Kevin there's still a bug regarding long/doubles handling:
On commit 49782161, line 101 of the diff, you're emitting gen.pop() if the expression is in STATEMENT position, you need to emit gen.pop2() instead if e.getReturnType is long.class or double.class

Test case:

user=> (fn [] (try 1 (finally)) 2)
VerifyError (class: user$eval1$fn__2, method: invoke signature: ()Ljava/lang/Object;) Attempt to split long or double on the stack  user/eval1 (NO_SOURCE_FILE:1)
Comment by Kevin Downey [ 26/Jul/14 1:46 AM ]

bah, all that work to figure out the thing I couldn't get right and of course I overlooked the thing I knew at the beginning. I want to get rid of some of the code duplication between emit and emitUnboxed for TryExpr, so when I get that done I'll fix the pop too

Comment by Kevin Downey [ 26/Jul/14 12:52 PM ]

hoistedmethod-pass-4.diff logically has the same three commits, but fixes the pop vs pop2 issue and rewrites emit and emitUnboxed for TryExpr to share most of their code

Comment by Kevin Downey [ 26/Jul/14 12:58 PM ]

hoistedmethod-pass-5.diff fixes a stupid mistake in the tests in hoistedmethod-pass-4.diff

Comment by Nicola Mometto [ 21/Aug/15 1:03 PM ]

Kevin Downey FWIW the patch no longer applies

Comment by Michael Blume [ 21/Aug/15 3:06 PM ]

A naive attempt to bring the patch up to date results in

compile-clojure:
     [java] Exception in thread "main" java.lang.ExceptionInInitializerError
     [java] 	at clojure.lang.Compile.<clinit>(Compile.java:29)
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7360)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7341)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
     [java] 	at clojure.lang.Compiler.access$300(Compiler.java:38)
     [java] 	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:589)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7112)
     [java] 	at clojure.lang.Compiler.eval(Compiler.java:7416)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7859)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:372)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:363)
     [java] 	at clojure.lang.RT.load(RT.java:453)
     [java] 	at clojure.lang.RT.load(RT.java:419)
     [java] 	at clojure.lang.RT.doInit(RT.java:461)
     [java] 	at clojure.lang.RT.<clinit>(RT.java:331)
     [java] 	... 1 more
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod
     [java] 	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4466)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7351)
     [java] 	... 17 more
Comment by Kevin Downey [ 25/Aug/15 1:00 PM ]

the pass 6 patch builds cleanly on 1d5237f9d7db0bc5f6e929330108d016ac7bf76c(HEAD of master as of this moment) and runs clojure's tests. I have not verified it against other projects as I did with the previous patches (I don't remember how I did that)

Comment by Michael Blume [ 25/Aug/15 1:38 PM ]

I get the same error I shared before applying the new patch to 1d5237f

Comment by Alex Miller [ 25/Aug/15 1:56 PM ]

I get some whitespace warnings with hoistedmethod-pass-6.diff but the patch applies. On a compile I get:

compile-clojure:
     [java] Exception in thread "main" java.lang.ExceptionInInitializerError
     [java] 	at clojure.lang.Compile.<clinit>(Compile.java:29)
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7362)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7156)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7343)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7156)
     [java] 	at clojure.lang.Compiler.access$300(Compiler.java:38)
     [java] 	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:588)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7355)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7156)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7114)
     [java] 	at clojure.lang.Compiler.eval(Compiler.java:7418)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7861)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:372)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:363)
     [java] 	at clojure.lang.RT.load(RT.java:453)
     [java] 	at clojure.lang.RT.load(RT.java:419)
     [java] 	at clojure.lang.RT.doInit(RT.java:461)
     [java] 	at clojure.lang.RT.<clinit>(RT.java:331)
     [java] 	... 1 more
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod
     [java] 	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4468)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353)
     [java] 	... 17 more
Comment by Nicola Mometto [ 25/Aug/15 2:36 PM ]

Looks like direct linking interacts with the diffs in this patch in non trivial ways.

Comment by Kevin Downey [ 25/Aug/15 3:05 PM ]

I must have screwed up running the tests some how, I definitely get the same error now

Comment by Kevin Downey [ 25/Aug/15 4:00 PM ]

after you get past the cast issuing (just adding some conditional logic there)

it looks like HoistedMethodInvocationExpr needs to be made aware of if it is emitting in an instance method or a static method, and do the right thing with regard to this pointers and argument offsets. this will likely require HoistedMethod growing the ability to be a static method (and maybe preferring static methods when possible).

if you cause HoistedMethod to set usesThis to true on methods that use it, then everything appears hunky-dory (if I ran the tests correctly), but this largely negates the new direct linking stuff, which is not good.

Comment by Kevin Downey [ 27/Aug/15 9:30 PM ]

hoistedmethod-pass-7.diff adds a single commit to hoistedmethod-pass-5.diff

the single commit changes hoisted methods to always be static methods, and adjusts the arguments in the invocation of the hoisted method based on if the containing function is a static/direct function or not.

again I haven't done the extended testing with this patch.

here is an example of what the hoisted methods look like

user> (println (disassemble (fn ^long [^long y] (let [x (try (/ 1 0) (catch Throwable t 0))] x))))                                                                       

// Compiled from form-init3851661302895745152.clj (version 1.5 : 49.0, super bit)                                                                                        
public final class user$eval1872$fn__1873 extends clojure.lang.AFunction implements clojure.lang.IFn$LL {                                                                
                                                                                                                                                                         
  // Field descriptor #9 Lclojure/lang/Var;                                                                                                                              
  public static final clojure.lang.Var const__0;                                                                                                                         
                                                                                                                                                                         
  // Field descriptor #11 Ljava/lang/Object;                                                                                                                             
  public static final java.lang.Object const__1;                                                                                                                         
                                                                                                                                                                         
  // Field descriptor #11 Ljava/lang/Object;                                                                                                                             
  public static final java.lang.Object const__2;                                                                                                                         
                                                                                                                                                                         
  // Method descriptor #14 ()V                                                                                                                                           
  // Stack: 2, Locals: 0                                                                                                                                                 
  public static {};                                                                                                                                                      
     0  ldc <String "clojure.core"> [16]                                                                                                                                 
     2  ldc <String "/"> [18]                                                                                                                                            
     4  invokestatic clojure.lang.RT.var(java.lang.String, java.lang.String) : clojure.lang.Var [24]                                                                     
     7  checkcast clojure.lang.Var [26]                                                                                                                                  
    10  putstatic user$eval1872$fn__1873.const__0 : clojure.lang.Var [28]                                                                                                
    13  lconst_1    
    14  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [34]                                                                                                  
    17  putstatic user$eval1872$fn__1873.const__1 : java.lang.Object [36]                                                                                                
    20  lconst_0                                                                                                                                                         
    21  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [34]                                                                                                  
    24  putstatic user$eval1872$fn__1873.const__2 : java.lang.Object [38]                                                                                                
    27  return                                                                                                                                                           
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
                                                                                                                                                                         
  // Method descriptor #14 ()V                                                                                                                                           
  // Stack: 1, Locals: 1                                                                                                                                                 
  public user$eval1872$fn__1873();                                                                                                                                       
    0  aload_0 [this]                                                                                                                                                    
    1  invokespecial clojure.lang.AFunction() [41]                                                                                                                       
    4  return                                                                                                                                                            
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
                                                                                                                                                                         
  // Method descriptor #43 (J)J                                                                                                                                          
  // Stack: 2, Locals: 4  
  public final long invokePrim(long y);                                                                                                                                  
     0  lload_1 [y]                                                                                                                                                      
     1  invokestatic user$eval1872$fn__1873.__hoisted1874(long) : java.lang.Object [47]                                                                                  
     4  astore_3 [x]                                                                                                                                                     
     5  aload_3 [x]                                                                                                                                                      
     6  aconst_null                                                                                                                                                      
     7  astore_3                                                                                                                                                         
     8  checkcast java.lang.Number [50]                                                                                                                                  
    11  invokevirtual java.lang.Number.longValue() : long [54]                                                                                                           
    14  lreturn                                                                                                                                                          
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
      Local variable table:                                                                                                                                              
        [pc: 5, pc: 8] local: x index: 3 type: java.lang.Object                                                                                                          
        [pc: 0, pc: 14] local: this index: 0 type: java.lang.Object                                                                                                      
        [pc: 0, pc: 14] local: y index: 1 type: long                                                                                                                     
                                                                                                                                                                         
  // Method descriptor #59 (Ljava/lang/Object;)Ljava/lang/Object;                                                                                                        
  // Stack: 5, Locals: 2                                                                                                                                                 
  public java.lang.Object invoke(java.lang.Object arg0);                                                                                                                 
     0  aload_0 [this]                                                                                                                                                   
     1  aload_1 [arg0]                                                                                                                                                   
     2  checkcast java.lang.Number [50]                                                                                                                                  
     5  invokestatic clojure.lang.RT.longCast(java.lang.Object) : long [63]                                                                                              
     8  invokeinterface clojure.lang.IFn$LL.invokePrim(long) : long [65] [nargs: 3]                                                                                      
    13  new java.lang.Long [30]                                                                                                                                          
    16  dup_x2                                                                                                                                                           
    17  dup_x2                                                                                                                                                           
    18  pop                                                                                                                                                              
    19  invokespecial java.lang.Long(long) [68]                                                                                                                          
    22  areturn                                                                                                                                                          
                                                                                                                                                                         
                                                                                                                                                                         
  // Method descriptor #45 (J)Ljava/lang/Object;                                                                                                                         
  // Stack: 4, Locals: 4                                                                                                                                                 
  public static java.lang.Object __hoisted1874(long arg0);                                                                                                               
     0  lconst_1                                                                                                                                                         
     1  lconst_0                                      
                                                                                                                                                        
     2  invokestatic clojure.lang.Numbers.divide(long, long) : java.lang.Number [74]                                                                                     
     5  astore_2                                                                                                                                                         
     6  goto 17                                                                                                                                                          
     9  astore_3 [t]                                                                                                                                                     
    10  getstatic user$eval1872$fn__1873.const__2 : java.lang.Object [38]                                                                                                
    13  astore_2                                                                                                                                                         
    14  goto 17                                                                                                                                                          
    17  aload_2                                                                                                                                                          
    18  areturn                                                                                                                                                          
      Exception Table:                                                                                                                                                   
        [pc: 0, pc: 6] -> 9 when : java.lang.Throwable                                                                                                                   
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
        [pc: 2, line: 1]                                                                                                                                                 
      Local variable table:                                                                                                                                              
        [pc: 9, pc: 14] local: t index: 3 type: java.lang.Object                                                                                                         
        [pc: 0, pc: 18] local: y index: 1 type: java.lang.Object                                                                                                         
                                                                                                                                                                         
}                           

Comment by Kevin Downey [ 27/Aug/15 9:43 PM ]

there is still an issue with patch 7, (defn f [^long y] (let [x (try (+ 1 0) (catch Throwable t y))] x)) causes a verifier error

Comment by Nicola Mometto [ 28/Aug/15 11:41 AM ]

Patch 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch is patch hoistedmethod-pass-7 with the following changes:

  • fixes the bug mentioned in the last comment by Kevin Downey
  • removes unnecessary whitespace and indentation changes
  • conforms indentation with the surrounding lines
  • squashes the commits into one as preferred by Rich

Authorship is maintained for Kevin Downey

Comment by Kevin Downey [ 28/Aug/15 5:40 PM ]

maybe we need a "Bronsa's Guide to Submitting Patches" to supplement http://dev.clojure.org/display/community/Developing+Patches, I had no idea a single commit was preferred, but that makes sense given the format, although I just noticed the bit on deleting old patches to avoid confusion. Is there a preferred format for patch names too?
If you have recommendations beyond patch formatting into the code itself I am all ears.

Comment by Nicola Mometto [ 28/Aug/15 6:08 PM ]

Kevin, having a single commit per patch is something that I've seen Rich and Alex ask for in a bunch of tickets, as I guess it makes it easier to evaluate the overall diff (even though it sacrifices granularity of description).
No idea for a preferred patch name, I find it hard to imagine it would matter – I just use whatever git format patch outputs.

One thing I personally prefer is to add the ticket name at the beginning of the commit message, it makes it easier to understand changes when using e.g. git blame

Comment by Andy Fingerhut [ 28/Aug/15 6:33 PM ]

Just now I added a suggestion to http://dev.clojure.org/display/community/Developing+Patches that one read their patches before attaching them, and remove any spurious white space changes. Also to consider submitting patches with a single commit, rather than ones broken up into multiple commits, as reviewers tend to prefer those.

Alex Miller recently edited that page with the note about putting the ticket id first in the commit comment.

Only preference on patch file names is the one on that page – that they end with '.patch' or '.diff', because Rich's preferred editor for reading them recognizes those suffixes and displays the file in a patch-specific mode, I would guess.

Comment by Nicola Mometto [ 28/Aug/15 6:38 PM ]

0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch is the same as 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch but it changes some indentation to avoid mixing tabs and spaces

Comment by Michael Blume [ 01/Sep/15 5:27 PM ]

I have a verify error on the latest patch, will attempt to provide a small test case:

Exception in thread "main" java.lang.VerifyError: (class: com/climate/scouting/homestead_test$fn__17125, method: invokeStatic signature: (Ljava/lang/Object;)Ljava/lang/Object Expecting to find integer on stack, compiling:(homestead_test.clj:43:3)

Comment by Michael Blume [ 01/Sep/15 6:29 PM ]

https://github.com/MichaelBlume/hoist-fail

I'd rather remove the dependency on compojure-api but I'm having trouble reproducing without it

Comment by Michael Blume [ 01/Sep/15 8:01 PM ]

Ok, less stupid test case:

(let [^boolean foo true]
  (do
    (try foo
      (catch Throwable t))
    nil))
Comment by Michael Blume [ 01/Sep/15 8:30 PM ]

...I wonder how hard it would be to write a generative test which produced small snippets of valid Clojure code, compiled them, and checked for errors. I bet we could've caught this.

Comment by Nicola Mometto [ 02/Sep/15 12:53 AM ]

Michael Blume I'm not sure this should be considered a compiler bug.
Contrary to numeric literals, boolean literals are never emitted unboxed and type hints are not casts so

(let [^boolean a true])
is lying to the compiler telling it a is an unboxed boolean when infact it is a boxed Boolean.

I'd say we wait for what Alex Miller or Rich Hickey think of this before considering Kevin Downey's current patch bugged, I personally (given the current compiler implementation) would consider this an user-code bug, currently ignored, that this patch merely exposes. IOW an instance of currently working code that is not however valid code

(I rest my case that if we had some better spec on what type-hints are supposed to be valid and some better compile-time validation on them, issues like this would not arise)

Comment by Nicola Mometto [ 02/Sep/15 1:13 AM ]

If this is considered a bug, the fix is trivial btw

@@ -5888,7 +5888,7 @@ public static class LocalBinding{
 
 	public boolean hasJavaClass() {
 		if(init != null && init.hasJavaClass()
-		   && Util.isPrimitive(init.getJavaClass())
+		   && (Util.isPrimitive(init.getJavaClass()) || Util.isPrimitive(tag))
 		   && !(init instanceof MaybePrimitiveExpr))
 			return false;
 		return tag != null
Comment by Kevin Downey [ 03/Sep/15 11:22 AM ]

I imagine ^boolean type hints (that don't do anything and are ignored) are going to become very common in clojure code, given everyone's keen interest in code sharing between clojure and clojurescript, and iirc clojurescript actually using ^boolean

Comment by Nicola Mometto [ 12/Sep/15 5:33 AM ]

Last patch doesn't compile anymore since it hits the bug reported in CLJ-1809

Comment by Alex Miller [ 18/Sep/15 8:28 AM ]

Moving to incomplete for now since it seems to be blocked on the other ticket

Comment by Kevin Downey [ 19/Jan/16 4:06 PM ]

0002-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch makes the change that Nicola suggested (with an extra null check). Michael's test case with the ^boolean type hint compiles now

Comment by Michael Blume [ 19/Jan/16 6:01 PM ]

Maybe this is out of scope for this ticket since it's just existing code that you're moving around, but I've always been confused by

//exception should be on stack
gen.visitVarInsn(OBJECT_TYPE.getOpcode(Opcodes.ISTORE), finallyLocal);
finallyExpr.emit(C.STATEMENT, objx, gen);
gen.visitVarInsn(OBJECT_TYPE.getOpcode(Opcodes.ILOAD), finallyLocal);

If we emit finallyExpr as C.STATEMENT, it should have no impact on the stack, and the exception should just be there without us needing to store and load it, right?

Comment by Michael Blume [ 19/Jan/16 6:02 PM ]

Also the latest patch seems to add a patch file to the root directory

Comment by Michael Blume [ 19/Jan/16 7:01 PM ]

New failing code

(defn foo [req]
  (let [^boolean b (get req :is-baz)]
    (do
      (try
        :bar)
      :foo)))
Comment by Kevin Downey [ 20/Jan/16 12:05 AM ]

the latest patch applied to master is causing some test failures for data.xml

Testing clojure.data.xml.test-seq-tree

FAIL in (release-head-top) (test_seq_tree.clj:52)
expected: (= nil (.get input-ref))
  actual: (not (= nil (0 1 2 3 4 5 6 7 8 9)))

FAIL in (release-head-nested-late) (test_seq_tree.clj:60)
expected: (= nil (.get input-ref))
  actual: (not (= nil (1 2 :< 3 4 5 :>))
Comment by Nicola Mometto [ 20/Jan/16 5:54 AM ]

Michael Blume that's still an invalid type hint. Clojure is inconsistent all over the place with enforcing valid type hints, so even though I think we should handle the VerifyError explicitly, I don't think we need to care about making that code work.

WRT ^boolean type hints going to be common because clojurescript uses them – I don't think we should accept invalid code for the sake of interoparability. See CLJ-1883 for an instance of such a wrong type hint being used in Om, the solution was to fix Om, not to make the clojure compiler ignore yet another wrong type hint.

Comment by Kevin Downey [ 19/Apr/16 1:17 PM ]

I've just got around to trying to dig in to the data.xml failures I mentioned above.

Both those tests are related to head holding on a lazy seq. Both tests reliably fail with 0002-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch

So I suspect the hoisted method isn't properly clearing locals.

Comment by Kevin Downey [ 19/Apr/16 5:56 PM ]

so I don't forget, I realized the issue with data.xml is because the 'is' macro in the test expands in to a try/catch in an expression context, which is hoisted, and the hoist causes the whole environment not to be cleared until the hoisted method returns, which is obviously not correct.





[CLJ-2179] [spec] s/inst-in and s/int-in generators should have uniform distribution not biased towards min value Created: 06/Jun/17  Updated: 08/Oct/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: gen-class, spec

Attachments: Text File clj-2179.patch    
Patch: Code
Approval: Incomplete

 Description   

The s/inst-in and s/int-in generators are based on gen/large-integer* which grows from 0.

(require '[clojure.spec.alpha :as s] '[clojure.spec.gen.alpha :as gen])
(gen/sample (s/gen (s/int-in 0 100)))
;;=> (1 0 1 1 1 0 1 1 72 1)

(gen/sample (s/gen (s/inst-in #inst "2001-01-01" #inst "2001-12-31")))
;;=> (#inst "2001-01-01T00:00:00.000-00:00" #inst "2001-01-01T00:00:00.000-00:00" #inst "2001-01-01T00:00:00.001-00:00" #inst "2001-01-01T00:00:00.001-00:00" ...)

Proposed: Instead, s/inst-in should use a uniform distribution generator:

After on same:

(26 16 65 96 63 37 31 4 94 9)

(#inst "2001-03-03T04:51:43.702-00:00" 
 #inst "2001-07-25T07:13:03.224-00:00" 
 #inst "2001-03-31T18:28:41.625-00:00" 
 #inst "2001-04-17T19:33:14.176-00:00" 
 #inst "2001-01-14T07:03:08.521-00:00" 
 #inst "2001-06-06T09:52:03.421-00:00" ...)

Patch: clj-2179.patch



 Comments   
Comment by Gary Fredericks [ 13/Nov/17 6:54 AM ]

What problem is this trying to solve?

Comment by Alex Miller [ 13/Nov/17 5:26 PM ]

Typically I find having the values biased towards the min value of the range (particularly in the inst case where values have to grow a lot to seem different) to not be what I expect as a user.

But I think the question is what the intended behavior should be for range specs. Whether or not Rich and Stu agree, I don't know yet.

Comment by Gary Fredericks [ 13/Nov/17 6:39 PM ]

For inst, I'd recommend at least generating the components of the timestamp separately (year, month, day, hour, etc.) and combining them with gen/fmap. That makes it shrink more naturally, and makes it easier to specify whatever strategy you like for biasing toward the present.

W.r.t. int ranges, I will only point out that one of test.check's features is to start tests with "small" values, so to whatever extent you impose uniform distributions, you neutralize that feature.

Comment by Gary Fredericks [ 13/Nov/17 6:43 PM ]

if you'd like a generator that starts out at both the min and the max and can shrink to either, something like this should work:

(defn bi-biased-int-range
  [min max]
  (let [g (gen/large-integer* {:min min, :max max})
        g' (gen/let [x g] (- max (- x min)))]
    (gen/one-of [g g'])))
Comment by Stuart Halloway [ 22/Jun/18 3:16 PM ]

Maybe these two cases need separate consideration. I like Gary's idea for timestamp and think that int generators should be left as-is.





[CLJ-2080] [spec] clojure.spec/every-kv does not work on vectors - improve docs/errors Created: 08/Dec/16  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

alpha 14


Attachments: Text File clj-2080-2.patch     Text File clj-2080-3.patch     Text File clj-2080-4.patch     Text File clj-2080-5.patch     Text File clj-2080-6.patch     Text File clj-2080-7.patch     Text File clj-2080-8.patch     Text File clj-2080.patch    
Patch: Code
Approval: Incomplete

 Description   

The every-kv doc states "takes separate key and val preds and works on associative collections". Vectors return true for associative? but do not currently work:

user=> (s/conform (s/every-kv any? any?) [])
[]
user=> (s/conform (s/every-kv any? any?) [1 2 3])
:clojure.spec/invalid
user=> (s/conform (s/every-kv integer? string?) [])
[]
user=> (s/conform (s/every-kv integer? string?) ["x"])
:clojure.spec/invalid

Another similar problem:

(s/explain-data (s/every-kv int? int?) [{:a :b}])
UnsupportedOperationException nth not supported on this type: PersistentArrayMap  clojure.lang.RT.nthFrom (RT.java:903)

Cause: Vectors should not work with every-kv. The combination of every-kv and every-impl assume that the collection passed to every-kv can provide a seq of map entries. In the explain case, the ::kfn created by every-kv is used to create a better path segment using the key rather than the element index. The kfn assumes that an element of the collection can call `(nth entry 0)` on the element. In the explain failure above, the map {:a :b} will throw when invoked with nth.

Proposed: Do the following to be clearer about the requirement that the coll elements are map entries:

  • Modify the docstring to say "seqs to map entries" rather than "is a map"
  • Modify the kfn to add a check that the element is an entry and if so, use it's key. If not, use the index (of the element itself). In this case, when passed an entry it will report with the actual key but when passed something that's not an entry, it will report the collection based index of the non-entry.

After the patch, the explain-data call will provide a useful error rather than the exception above:

user=> (s/explain-data (s/every-kv int? int?) [{:a :b}])
#:clojure.spec.alpha{:problems ({:path [], :pred vector?, :val {:a :b}, :via [], :in [0]})}

Patch: clj-2080-8.patch



 Comments   
Comment by Alex Miller [ 09/Dec/16 8:35 AM ]

At the moment, I'm inclined to say the doc in every-kv should be tightened to say "map" instead of "associative collection" but will check with Rich.

Comment by Alex Miller [ 31/Mar/17 9:07 AM ]

Updated to apply to master

Comment by Alex Miller [ 10/May/17 12:26 PM ]

Updated patch to apply to spec.alpha.

Comment by Stuart Halloway [ 12/May/17 10:08 AM ]

Can you please update this ticket with expected behavior for the examples shown in the description, plus test showing that expected behavior. I am still seeing vectors fail for conform.

(s/explain-data (s/every-kv integer? string?) ["x"])
=> #:clojure.spec.alpha{:problems ({:path [], :pred vector?, :val "x", :via [], :in [0]})}
Comment by Alex Miller [ 12/May/17 10:33 AM ]

Vectors should be failing - every-kv requires a seq of map entries. The patch clarifies this in the docstring and fixes the exception that can (incorrectly) be thrown when producing explain-data. I updated the title, the description to include the explain-data after behavior, and added a test to the patch.

Comment by Alex Miller [ 12/May/17 10:52 AM ]

Lost docstring change a ways back, re-added in -7 patch.

Comment by Alex Miller [ 18/Jun/18 10:45 AM ]

-8 updates to apply to master, no semantic changes





[CLJ-2067] [spec] (s/def ::a ::b) throws unable to resolve error if ::b is not defined Created: 22/Nov/16  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Oliver George Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec

Approval: Vetted

 Description   

It should be possible to do `(s/def ::a ::b)` before declaring ::b.

Currently this throws an "Unable to resolve" error.

Alex indicated that everything should have delays but that they are missing in some cases. This seems like one of those cases.

Examples where things work fine are `(s/def ::a (s/and ::b))` and `(s/def ::a (s/keys :req [::b]))`.






[CLJ-2041] [spec] clojure.spec/keys requires input collections conform to clojure.core/map? Created: 11/Oct/16  Updated: 08/Oct/18

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

Type: Defect Priority: Minor
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 10
Labels: spec

Approval: Vetted

 Description   

I would like to use specs to validate Datomic entities. However, `s/keys` is too restrictive in that it requires input collections to conform to `clojure.core/map?` instead of some more primitive interface (for example clojure.lang.ILookup or clojure.lang.Associative).



 Comments   
Comment by Alex Miller [ 04/Nov/16 8:21 AM ]

s/keys uses IPersistentMap's Iterable support for iterating through all entries for validation. ILookup and Associative do not support iteration. So, that's why it is the way it is. But, understand the desire.

Comment by Alex Miller [ 04/Nov/16 8:34 AM ]

Datomic entities are seqable so maybe that's a potential path (would be slower for actual PHMs though).

Comment by Odin Standal [ 04/Nov/16 8:35 AM ]

Thanks for following up. So any ideas or guidance on how to use clojure.spec with Datomic entities?

Comment by Alex Miller [ 04/Nov/16 8:37 AM ]

For now, you could use into to pour an entity into a PHM before validating. I hesitate to suggest it, but that could even be in the spec with a leading conformer.

Comment by Alex Miller [ 14/Nov/16 12:16 PM ]

Moving this into 1.9 for the moment just so we don't lose it. Not sure whether we can or will actually do anything with this though.

Comment by Alex Miller [ 08/Mar/17 10:00 AM ]

This is related to CLJ-2080 as it's the same basic issue of what map-of and every-kv actually expect as valid inputs, which is: something that seqs to map entries. We can't really write a predicate for it without an interface "ISeqsToMapEntries" (intentionally bad name) to indicate this. java.util.Map, IPersistentMap, etc imply this, but an object can seq to map entries without meeting all the constraints of those much broader interfaces. ILookup does not and should not imply this.





[CLJ-1986] Suppress printing namespace map literal syntax when only one namespaced key Created: 21/Jul/16  Updated: 08/Oct/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: maps, print

Attachments: Text File clj-1986.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Really an aesthetic choice, but right now maps with only a single namespaced key are printed in namespace map literal syntax:

user=> {:my.ns/b 1}
#:my.ns{:b 1}

And that seems unnecessarily complicated (and longer).

Proposal: Only print namespace map literal syntax when >1 key is using the same namespace.

Patch: clj-1986.patch






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

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

Type: Enhancement Priority: Minor
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ft

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

 Description   

Two performance enhancements on clojure.set/map-invert:

1) Use reduce-kv to avoid realizing mapentry's from input map
2) Use transients to create the output map

Patch: clj-1808-map-invert-should-use-reduce-kv-and-transient.patch



 Comments   
Comment by Alex Miller [ 04/Sep/15 10:42 AM ]

Would be nice to see a quick perf test that compared before/after times.





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

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

Type: Defect Priority: Minor
Reporter: Kevin Woram Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: checkargs, newbie

Attachments: Text File clj-1647_2.patch     Text File clj-1647_3.patch     Text File clj-1647.patch     Text File kworam-clj-1647.patch    
Patch: Code and Test
Approval: Vetted

 Description   

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

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

Cause: partition and partition-all do not check that n and step are positive.

Approach: Add checks to partition and partition-all that n and step are positive.

Patch: clj-1647_3.patch

Prescreened by: Alex Miller



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

Also see CLJ-764

Comment by Alex Miller [ 29/Apr/15 12:02 PM ]

Needs a perf check when done.

Comment by Kevin Woram [ 16/May/15 1:58 PM ]

patch file to fix clj-1647

Comment by Kevin Woram [ 16/May/15 2:19 PM ]

Since 'n' and 'step' remain unchanged from the initial function call through all of the recursive self-calls, I only need to verify that they are positive once, on the initial call.

I therefore created functions 'internal-partition' and 'internal-partition-all' whose implementations are identical to the current versions of 'partition' and 'partition-all'.

I then added preconditions that 'step' and 'n' must be positive to the 'partition' and 'partition-all' functions, and made them call 'internal-partition' and 'internal-partition-all' respectively to do the work.

Comment by Alex Miller [ 17/May/15 8:14 AM ]

There are a lot of unrelated whitespace changes in this patch - can you supply a smaller patch with only the change at issue? Also needs tests.

Comment by Kevin Woram [ 17/May/15 2:05 PM ]

I will supply a patch file without the whitespace changes.

I know there are some existing functionality tests for 'partition' and 'partition-all' in test_clojure\sequences.clj and test_clojure\transducers.clj. I don't think I need to add more functionality tests, but I think I should add:

1. Tests that verify that non-positive 'step' and 'n' parameters are rejected.
2. Tests that show that 'partition' and 'partition-all' performance has not degraded significantly.

Could you give me some guidance on how to develop and add these tests?

Comment by Alex Miller [ 17/May/15 3:31 PM ]

You should add #1 to the patch. For #2, you can just do the timings before/after (criterium is a good tool for this) and put the results in the description.

Comment by Kevin Woram [ 22/May/15 4:04 PM ]

I have coded up the tests for #1 and taken some 'before' timings for #2 using criterium.

I have been stumped by a problem for hours now and I need to get some help. I made my changes to 'partition' and 'partition-all' in core.clj and then did 'mvn package' to build the jar. I executed 'target>java -cp clojure-1.7.0-master-SNAPSHOT.jar clojure.main' to test out my patched version of clojure interactively. The (source ...) function shows that my source changes for both 'partition' and 'partition-all' are in place. My change to 'partition-all' seems to be working but my change to 'partition' is not. As far as I can tell, they should both throw an AssertionError with the input parameters I am providing.

Any help would be greatly appreciated.

user=> (source partition)
(defn partition
"Returns a lazy sequence of lists of n items each, at offsets step
apart. If step is not supplied, defaults to n, i.e. the partitions
do not overlap. If a pad collection is supplied, use its elements as
necessary to complete last partition upto n items. In case there are
not enough padding elements, return a partition with less than n items."
{:added "1.0"
:static true}
([n coll]
{:pre [(pos? n)]}
(partition n n coll))
([n step coll]
{:pre [(pos? n) (pos? step)]}
(internal-partition n step coll))
([n step pad coll]
{:pre [(pos? n) (pos? step)]}
(internal-partition n step pad coll)))
nil
user=> (partition -1 [1 2 3])
()
user=> (source partition-all)
(defn partition-all
"Returns a lazy sequence of lists like partition, but may include
partitions with fewer than n items at the end. Returns a stateful
transducer when no collection is provided."
{:added "1.2"
:static true}
([^long n]
(internal-partition-all n))
([n coll]
(partition-all n n coll))
([n step coll]
{:pre [(pos? n) (pos? step)]}
(internal-partition-all n step coll)))
nil
user=> (partition-all -1 [1 2 3])
AssertionError Assert failed: (pos? n) clojure.core/partition-all (core.clj:6993)

Comment by Alex Miller [ 22/May/15 4:47 PM ]

Did you mvn clean? Or rm target?

Comment by Kevin Woram [ 24/May/15 11:46 PM ]

Yes, I did mvn clean and verified that clojure-1.7.0-master-SNAPSHOT.jar had the expected date-time stamp before doing the interactive test. I even went so far as to retrace my steps on my Macbook on the theory that maybe there was a Windows-specific build problem.

My change to partition-all works as expected but my change to partition does not. However, if I copy the result of the call to (source partition) and execute it (replacing clojure.core/partition with user/partition), user/partition works as expected. I don't understand why my change to clojure.core/partition isn't taking effect.

Comment by Andy Fingerhut [ 25/May/15 1:27 AM ]

Kevin, I do not know the history of your Clojure source tree, but if you ever ran 'ant' in it, that creates jar files in the root directory, whereas 'mvn package' creates them in the target directory. It wasn't clear from your longer comment above whether the 'java -cp ...' command you ran pointed at the one in the target directory. That may not be the cause of the issue you are seeing, but I don't yet have any guesses what else it could be.

Comment by Alex Miller [ 13/Jul/15 1:17 PM ]

What's the status of this?

Comment by Kevin Woram [ 16/Jul/15 10:20 PM ]

Alex, I moved to Seattle and took a permanent position with Microsoft recently. This has kept me very busy and I haven't been able to spend time on Clojure at all. I probably won't be able to devote time to Clojure for another month or two.

Comment by Alex Miller [ 16/Jul/15 10:46 PM ]

Thanks for the heads up.

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

Kevin, Alex, I could pick this up if you like?

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:41 PM ]

Sorry, browser fail

Comment by Kevin Woram [ 25/Jul/15 7:03 PM ]

Thanks for picking it up Matthew, I appreciate it!

Comment by Kevin Woram [ 25/Jul/15 7:03 PM ]

Thanks for picking it up Matthew, I appreciate it!

Comment by Matthew Gilliard [ 27/Jul/15 11:30 AM ]

New patch: clj-1647.patch

Includes tests, fewer whitespace changes, manually thrown IAEs. I'll do some basic benchmarking soon, although I expect the overhead to be quite low as we're only checking the arguments once.

Kevin, the reason your patch was working for partition-all but not partition is that partition is defined early-ish in the bootstrapping process and the {:pre .. :post ..} maps aren't read by defn until it's enhanced later on.

Comment by Kevin Woram [ 27/Jul/15 12:10 PM ]

Thanks for solving that mystery Matthew!

Comment by Alex Miller [ 10/Mar/16 2:45 PM ]

Patch looks basically good. Minor changes:

  • internal-partition and internal-partition-all should be marked private with defn-.
  • Commit description should start with "CLJ-1647"
Comment by Matthew Gilliard [ 26/Apr/16 10:32 AM ]

I added clj-1647_2.patch to supersede other patches on this issue. Jira ref is added to commit msg and defn- used where possible (defn- is not defined until after one of the private fns but there is the ^:private metadata added manually)

Comment by Alex Miller [ 26/Apr/16 11:42 AM ]

The patch changes add-annotation from defn- to defn but that seems unrelated to the intent of the patch?

Comment by Matthew Gilliard [ 27/Apr/16 3:15 AM ]

Thanks for looking so quickly Alex - sorry about that error in add-annotation. See clj-1647_3.patch





[CLJ-1385] Docstrings for `conj!` and `assoc!` should suggest using the return value; effect not always in-place Created: 16/Mar/14  Updated: 08/Oct/18

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

Type: Enhancement Priority: Minor
Reporter: Pyry Jahkola Assignee: Alex Miller
Resolution: Unresolved Votes: 5
Labels: collections, docstring, ft

Attachments: Text File CLJ-1385-reword-docstrings-on-transient-update-funct-2.patch     Text File CLJ-1385-reword-docstrings-on-transient-update-funct.patch    
Patch: Code
Approval: Incomplete

 Description   

The docstrings of both `assoc!` and `conj!` say "Returns coll.", possibly suggesting the transient edit happens (always) in-place, `coll` being the first argument. However, this is not the case and the returned collection should always be the one that's used.

Approach: Replace "Returns coll." with "Returns an updated collection." in `conj!`, `assoc!`, `pop!` docstrings.

Patch: CLJ-1385-reword-docstrings-on-transient-update-funct-2.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 16/Mar/14 8:49 AM ]

When modifying transient collections, it is required to use the collection returned from functions like assoc!. The ! here indicates its destructive nature. The transients page (http://clojure.org/transients) describes the calling pattern pretty explicitly: "You must capture and use the return value in the next call."

I do not agree that we should be guiding programmers away from using functions like assoc! – transients are used as a performance optimization and using assoc! or conj! in a loop is often the fastest version of that. However I do think it would be helpful to make the docstring more explicit.

Comment by Gary Fredericks [ 05/Apr/14 10:23 AM ]

Alex I think you must have misread the ticket – the OP is suggesting guiding toward using the return value of assoc!, not avoiding assoc! altogether.

And the docstring is not simply inexplicit, it's actually incorrect specifically in the case that the OP pointed out. conj! and assoc do not return coll at the point where array-maps transition to hash-maps, and the fact that they do otherwise is supposed to be an implementation detail as far as I understand it.

Comment by Alex Miller [ 05/Apr/14 11:55 AM ]

@Gary - you're right, I did misread that.

assoc and conj both explicitly say "return a new collection" whereas assoc! and conj! say "Returns coll." I read that as "returns the modified collection" without regard to whether it's the identical instance, but I can read it your way too.

Would saying "Returns updated collection." transmit the right idea? Using "collection" instead of "coll" removes the concrete tie to the variable and "updated" hints more strongly that you should use the return value.

Comment by Pyry Jahkola [ 05/Apr/14 12:47 PM ]

@Alex, that update makes it sound right to me, FWIW.

Comment by Gary Fredericks [ 05/Apr/14 2:37 PM ]

Yeah, I think that's better. Thanks Alex. I'd be happy to submit a patch for that but I'm assuming patches are too heavy for this kind of change?

Comment by Andy Fingerhut [ 06/Apr/14 3:35 PM ]

Patches are exactly what has been done in the past for this kind of change, if it is in a doc string and not on the clojure.org web page.

Comment by Alex Miller [ 06/Apr/14 4:13 PM ]

Yup, patch desired.

Comment by Gary Fredericks [ 06/Apr/14 5:32 PM ]

Glad I asked.

Patch is attached that also updates the docstring for pop! which had the same issue, though arguably it's less important since afaik pop! does always return the identical collection (but I don't think this is part of the contract).

Comment by Andy Fingerhut [ 06/Aug/14 2:14 PM ]

Patch CLJ-1385-reword-docstrings-on-transient-update-funct.patch dated Apr 6 2014 no longer applies to latest Clojure master cleanly, due to some changes committed earlier today. I suspect it should be straightforward to update the patch to apply cleanly, given that they are doc string changes, but there may have been doc string changes committed to master, too.

Comment by Gary Fredericks [ 06/Aug/14 3:04 PM ]

Attached a new patch.

Comment by Rich Hickey [ 09/Oct/15 8:04 AM ]

I think it could be clearer still, since we want people to know the original coll might have been affected and returned, and the return must be used for subsequent calls. I think some of the language from the transients page should make it into these docstrings.

Comment by Andy Fingerhut [ 24/Oct/15 2:25 PM ]

Would it be correct to say that the collection passed into pop! conj! assoc! etc. has undefined contents after the operation completes, and only the return value has defined contents?

That kind of strong wording may get people's attention.

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

I'm working on this.

Comment by Alex Miller [ 22/Sep/16 1:42 PM ]

incomplete and I (still) own this one





[CLJ-1381] Improve support for extending protocols to primitive arrays Created: 13/Mar/14  Updated: 08/Oct/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: protocols

Approval: Vetted

 Description   

It is possible to extend protocols to primitive arrays but specifying the class for the type is a little tricky:

(defprotocol P (p [_]))
(extend-protocol P (Class/forName "[B") (p [_] "bytes"))
(p (byte-array 0))   ;; => "bytes"

However, things go bad if you try to do more than one of these:

(extend-protocol P 
  (Class/forName "[B") (p [_] "bytes")
  (Class/forName "[I") (p [_] "ints"))
CompilerException java.lang.UnsupportedOperationException: nth not supported on this type: Character, compiling:(NO_SOURCE_PATH:1:1)
	clojure.lang.Compiler.analyze (Compiler.java:6380)
	clojure.lang.Compiler.analyze (Compiler.java:6322)
	clojure.lang.Compiler$MapExpr.parse (Compiler.java:2879)
	clojure.lang.Compiler.analyze (Compiler.java:6369)
	clojure.lang.Compiler.analyze (Compiler.java:6322)
	clojure.lang.Compiler$InvokeExpr.parse (Compiler.java:3624)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6562)
	clojure.lang.Compiler.analyze (Compiler.java:6361)
	clojure.lang.Compiler.analyze (Compiler.java:6322)
	clojure.lang.Compiler$BodyExpr$Parser.parse (Compiler.java:5708)
	clojure.lang.Compiler$FnMethod.parse (Compiler.java:5139)
	clojure.lang.Compiler$FnExpr.parse (Compiler.java:3751)
Caused by:
UnsupportedOperationException nth not supported on this type: Character
	clojure.lang.RT.nthFrom (RT.java:857)
	clojure.lang.RT.nth (RT.java:807)
	clojure.core/emit-hinted-impl/hint--5951/fn--5953 (core_deftype.clj:758)
	clojure.core/map/fn--4207 (core.clj:2487)
	clojure.lang.LazySeq.sval (LazySeq.java:42)
	clojure.lang.LazySeq.seq (LazySeq.java:60)
	clojure.lang.RT.seq (RT.java:484)
	clojure.lang.RT.countFrom (RT.java:537)
	clojure.lang.RT.count (RT.java:530)
	clojure.lang.Cons.count (Cons.java:49)
	clojure.lang.Compiler.analyze (Compiler.java:6352)
	clojure.lang.Compiler.analyze (Compiler.java:6322)

The code in {parse-impls} is seeing the second {(Class/forName "[I")} as a function, not as a new type. One workaround for this is to only extend the protocol to one type at a time.

It would be even better (moving into enhancement area) if there was a syntax here to specify primitive array types - we already have the syntax of {bytes, ints, longs}, etc for type hints and those seem perfectly good to me.



 Comments   
Comment by Nahuel Greco [ 18/Sep/14 6:08 PM ]

It also breaks when extending only one array type:

(extend-protocol P
  String               (p [_] "string")
  (Class/forName "[B") (p [_] "ints") 
  )

;=> CompilerException java.lang.UnsupportedOperationException: nth not supported on this type ...

But changing the declaration order fixes it:

(extend-protocol P
  (Class/forName "[B") (p [_] "ints") 
  String               (p [_] "string")
  )

;=> OK
Comment by Alex Miller [ 12/Jan/16 3:16 PM ]

Dupe of CLJ-1790

Comment by Alex Miller [ 07/Jun/16 4:00 PM ]

On further inspection, I don't think this is a dupe of CLJ-1790 but merely a related problem.

Comment by Greg Chapman [ 18/Mar/17 11:04 AM ]

Using Class/forName has a further problem, as type-hints on the this parameter are longer emitted:

user=> (extend-protocol P (Class/forName "[B") (p [this] (aget this 0)))
Reflection warning, NO_SOURCE_PATH:2:50 - call to static method aget on clojure.lang.RT can't be resolved (argument types: unknown, int).

Reflection warnings are also generated for non-primitive arrays (so just supporting ints etc, won't completely fix this problem). It would be good to have a solution which covered all the problems with extending protocols to arrays.





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

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

Type: Defect Priority: Major
Reporter: Chouser Assignee: Unassigned
Resolution: Unresolved Votes: 10
Labels: memory, protocols

Attachments: File naive-lru-for-multimethods-and-protocols.diff     File protocol_multifn_weak_ref_cache.diff    
Patch: Code
Approval: Incomplete

 Description   

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

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

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

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

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

Multimethod leak

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

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

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

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

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

(future (run-loop))

(reset! sleep-duration 0)

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

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

These lines will stop once the PermGen space fills up.

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

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

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

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

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

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

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

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

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

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

Protocol leak

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

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

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

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

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

(future (run-loop))

(reset! sleep-duration 0)

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

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

(-reset-methods ITake)

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

Patch: protocol_multifn_weak_ref_cache.diff

Screened by:



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

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

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

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

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

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

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

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

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

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

Comment by Kevin Downey [ 17/Apr/14 9:52 PM ]