<< Back to previous view

[CLJ-2013] spec doesn't explain failing path of a s/cat with purely optional branches Created: 24/Aug/16  Updated: 24/Aug/16

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

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

alpha11



 Description   

In an s/cat with two optional regex branches, e. g. via s/? or s/*, spec doesn't explain their individual problems, but the whole spec as failed.

(s/explain (s/cat :begin (s/? (s/cat :num number?))
                  :end (s/* #{:foo}))
           [:bar])

In: [0] val: (:bar) fails predicate: (cat :begin (? (cat :num number?)) :end (* #{:foo})),  Extra input

Spec does not explain the full optional paths that failed, but instead explains that the s/cat spec failed as a whole.

If one forces spec down into one branch, it explains the error at the deepest possible path and explains the failing predicate

(s/explain (s/cat :begin (s/? (s/cat :num number?))
               ;; :end (s/* #{:foo})
                  )
           [:bar])
In: [0] val: :bar fails at: [:begin :num] predicate: number?

An interesting case is if one makes the second branch non-optional

(s/explain (s/cat :begin (s/? (s/cat :num number?))
                  :end #{:foo})
           [:bar])
In: [0] val: :bar fails at: [:end] predicate: #{:foo}

It does not explain why the first branch has failed as a potential option, but only the second. This makes sense from the perspective that it successfully parses the :begin branch as legally non-existent and then explains a detailed failure on the second one. However it omits valuable information in real world use cases as shown in https://groups.google.com/d/msg/clojure/mIlKaOiujlo/tF71zZ2BCwAJ .

Desired behavior would be at least that if all branches are optional in a cat and all fail they are all reported.

At most that if a cat fails but an optional branch was parsed as non-existent it is retried without being allowed to be parsed as non-existent.






[CLJ-2012] ns spec breaks gen-class forms that use strings as class names Created: 24/Aug/16  Updated: 24/Aug/16

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: regression, spec

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

 Description   

The following valid `ns` gen-class form is reported as invalid by spec:

(ns foo
  (:gen-class
     :name ^{org.apache.logging.log4j.core.config.plugins.Plugin
             {:name "LogstashConverter" :category "Converter"}
             org.apache.logging.log4j.core.pattern.ConverterKeys ["logstashJson"]} social.common.logging.LogstashPatternConverter
     :constructors {["[Ljava.lang.String;"] [String String]}
     :factory newInstance
     :init init
     :state state
     :extends org.apache.logging.log4j.core.pattern.LogEventPatternConverter))

This is because the ns spec assumes that all class names can be represented by simple-symbols, while in reality some can only be represented by strings.






[CLJ-1988] collection specs conform to reversed list when used on a sequence Created: 24/Jul/16  Updated: 23/Aug/16

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

Type: Defect Priority: Minor
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File clj-1988.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Example

Clojure 1.9.0-alpha10
user=> (require '[clojure.spec :as s])
nil
user=> (s/conform (s/coll-of int?) (range 5))
(4 3 2 1 0)

Problem: Current code handles vectors, maps, and lists but falls through on sequences to the last case and adds to an empty sequence (at the head).

Approach: Add seqs to the list case.

Patch: clj-1988.patch



 Comments   
Comment by Alex Miller [ 23/Aug/16 2:01 PM ]

In case it helps, a workaround might be: (s/conform (s/coll-of int? :into ()) (range 5))





[CLJ-1989] `let` ported from `test.check/let` to `clojure.spec.gen` Created: 24/Jul/16  Updated: 23/Aug/16

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

Type: Enhancement Priority: Minor
Reporter: Matthew Wampler-Doty Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File gen-let.patch    
Patch: Code and Test

 Description   

When using `clojure.spec` for elaborate specifications and `clojure.spec.gen` for generative testing, developers often find themselves writing code which heavily relies on `clojure.spec.gen/fmap`. This is sometimes unnatural and difficult to read.

To make writing custom generators easier, this patch ports `test.check/let` to `clojure.spec.gen`. Now developers can write generators more simply.



 Comments   
Comment by Matthew Wampler-Doty [ 24/Jul/16 5:55 PM ]

For example, if a user wanted to make a generator of vectors with length between 5 and 11 or 20 to 40 elements, consisting of keywords which were either `:a` or `:b`, they would have to write something like:

(gen/fmap (fn [[n gens]] (take n gens)) 
          (gen/tuple (spec/gen (spec/or :short (int-in 5 11) 
                                        :long (int-in 20 40)))
                     (gen/vector (gen/elements #{:a :b}) 40)))

With this patch they could write this as:

(gen/let [length (spec/or :short (int-in 5 11)
                          :long  (int-in 20 40))]
  (repeat length #{:a :b}))




[CLJ-2006] clojure.spec/fdef mentions nonexistent clojure.spec.test/run-tests Created: 21/Aug/16  Updated: 23/Aug/16

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

Type: Defect Priority: Minor
Reporter: lvh Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, spec
Environment:

N/A


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

 Description   

Once registered, function specs are included in doc, checked by
instrument, tested by the runner clojure.spec.test/run-tests, and (if
a macro) used to explain errors during macroexpansion.

Should be: clojure.spec.test/check



 Comments   
Comment by lvh [ 21/Aug/16 6:55 AM ]

Crud, I messed up the title of this issue copy-pasting from the docstring, and I appear to lack the permissions to resolve it.

Comment by Alex Miller [ 21/Aug/16 3:34 PM ]

lvh I've added you to the groups with edit permission.





[CLJ-2008] clojure.spec.test/check without args tests fns from clojure.core, erroring out with #:clojure.spec{:failure :no-fn} Created: 21/Aug/16  Updated: 23/Aug/16

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

Type: Defect Priority: Major
Reporter: lvh Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

N/A


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

 Description   

When running (clojure.spec.test/check (in CIDER, but I doubt that's relevant other than it changes the formatting of the following snippet):

0. { :failure clojure.lang.ExceptionInfo: No fn to spec #:clojure.spec{:failure :no-fn}, :sym clojure.core/when-let, :spec clojure.spec$fspec_impl$reify__13891@6ee9efaf }
  1. { :failure clojure.lang.ExceptionInfo: No fn to spec #:clojure.spec{:failure :no-fn}, :sym clojure.core/let, :spec clojure.spec$fspec_impl$reify__13891@4ff7da85 }
  2. { :failure clojure.lang.ExceptionInfo: No fn to spec #:clojure.spec{:failure :no-fn}, :sym clojure.core/if-let, :spec clojure.spec$fspec_impl$reify__13891@35daec9c }
  3. { :failure clojure.lang.ExceptionInfo: No fn to spec #:clojure.spec{:failure :no-fn}, :sym clojure.core/fn, :spec clojure.spec$fspec_impl$reify__13891@98fcf1f }
... (some elided)
  8. { :failure clojure.lang.ExceptionInfo: No fn to spec #:clojure.spec{:failure :no-fn}, :sym clojure.core/ns, :spec clojure.spec$fspec_impl$reify__13891@283add16 }
  9. { :failure clojure.lang.ExceptionInfo: No fn to spec #:clojure.spec{:failure :no-fn}, :sym clojure.core/defn, :spec clojure.spec$fspec_impl$reify__13891@59681966 }
... (some elided)
  12. { :failure clojure.lang.ExceptionInfo: No fn to spec #:clojure.spec{:failure :no-fn}, :sym clojure.core/defn-, :spec clojure.spec$fspec_impl$reify__13891@3b34b0db }

It doesn't seem appropriate to tell me that these macros have no fn to spec. Perhaps it should know about macros.

Problem: checkable-syms does not omit spec'ed macros so when they flow down to check-1, they throw errors. Since these can't be checked, they should be omitted from checkable-syms. I put the change in fn-spec-name?, which is also used (and has the same problem in) instrumentable-syms.

Approach: Skip spec'ed macros.

Patch: clj-2008.patch



 Comments   
Comment by Alex Miller [ 22/Aug/16 10:10 AM ]

That's interesting as there were changes in alpha11 designed to catch and omit macros from check. So, something still amiss there.





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

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: 0
Labels: spec


 Description   

Example, using 1.9.0-alpha8, same on master:

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/? (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--13632/fn--13638 (spec.clj:1066)
       	clojure.spec/every-impl/reify--13649 (spec.clj:1077)
       	clojure.spec/conform (spec.clj:117)

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






[CLJ-2009] 'symbol and 'keyword turn "" into unreadable symbol and keyword respectively Created: 22/Aug/16  Updated: 23/Aug/16  Resolved: 22/Aug/16

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

Type: Defect Priority: Major
Reporter: Aaron Brooks Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

In playing with clojure.spec for our upcoming Clojure meetup exercise and I found that (symbol "") returns an empty symbol which is unprintable and unreadable but can still be converted back to an empty string (name (symbol "")) => "". Similarly, (keyword "") returns ":" (which is invalid Clojure and cannot be read) but does round-trip as an object (name (keyword "")) => ""

I'm happy to provide a patch if we can determine the correct behavior. I'll start by making the assertion that the current behavior seems tricksy and prone to cause Great Mystery followed by Great Sadness.



 Comments   
Comment by Alex Miller [ 22/Aug/16 9:32 PM ]

Hey Aaron, symbols and keyword are programmatically constructable (and usable) without being constrained to what the reader can read and the printer can print, and that's often a useful feature (and something we don't consider to be a bug). It is possible that we might in the future have some kind of delimited keyword or symbol (| has been reserved for this) that would allow the reader to read these. I've done some research on this in the past and there were more complexities to it than initially thought so we ended up shelving it, but it's not dead, just in the deep freeze.

I've answered this question so many times that it's silly I haven't put it in the http://clojure.org/guides/faq, so I will do so for the next time.

I think when this comes up, what people most commonly want is some way to avoid making a non-readable keyword or symbol. So some kind of readable-keyword and readable-symbol functions that did validation and either threw an exception or created the keyword or symbol would maybe be a good enhancement idea.

Comment by Aaron Brooks [ 22/Aug/16 10:50 PM ]

Ships passing... I just saw the FAQ update and rechecked here.

The FAQ update is helpful. It might be worth noting in the docstrings of both 'symbol and 'keyword — that they are able to produce keywords and symbols which are unprintable/readable. I do also like the idea of readable-symbol and readable-keyword. Having a defined grammar for those would be great too.

Thanks for taking the time with this.

I am satisfied with my care. ;-D

Comment by Alex Miller [ 22/Aug/16 10:55 PM ]

If you want to file an enhancement for the readable-symbol/keyword, go for it. There is a grammar (kind of) on the http://clojure.org/reference/reader page, but it does not exactly line up with what's actually accepted in the regex, and there are several tickets already out there about the details of that misalignment. I think due to the intent that readable-symbol/keyword could take a conservative approach though and this would be pretty handy.

Comment by Aaron Brooks [ 22/Aug/16 11:03 PM ]

I meant grammar ala [E]BNF or similar. Something you can data rather than human. Also, as you note, the Java regex is pretty loose. I ran into this pretty directly when creating https://github.com/abrooks/clj-chopshop.

I've given some thought about proposing a grammar that would be used by the compiler (or other tools). Would patches for that be generally welcome or are we really attached to the regexes as they are? I understand that there are natural performance and compatibility concerns.

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

I don't think we're looking to replace the general reader strategy currently in use unless you could demonstrate significantly better performance.





[CLJ-2011] clojure.walk.macroexpand-all will not properly expand macros that depend on &env Created: 23/Aug/16  Updated: 23/Aug/16

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

Type: Defect Priority: Major
Reporter: Collin Bell Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: macro, walk
Environment:

MacOSX, Clojure 1.9.0-alpha10, Java 1.8.0_45, CIDER 0.13.0snapshot (package: 20160602.809), nREPL 0.2.12



 Description   

(clojure.walk/macroexpand-all '(defn foo [a] (go [] a)))

Unhandled clojure.lang.ExceptionInfo
Could not resolve var: a
{:var a}

This is because go depends on &env and macroexpand-all does not handle &env.

The reason this issue is important is because it breaks the cider debugger for async.






[CLJ-2010] clojure.spec/fdef does not add specs to doc Created: 22/Aug/16  Updated: 22/Aug/16  Resolved: 22/Aug/16

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

Type: Defect Priority: Major
Reporter: lvh Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: spec


 Description   

fdef docstring claims:

Once registered, function specs are included in doc, checked by
instrument, tested by the runner clojure.spec.test/run-tests, and (if
a macro) used to explain errors during macroexpansion.

When specifying an fdef for a fn, (:doc (meta my-fn) does not include any information about the spec, which is what I was expecting.



 Comments   
Comment by Alex Miller [ 22/Aug/16 10:52 PM ]

You are misinterpreting the intent there. What we mean there is that the clojure.repl/doc function will emit the spec as part of its output once you have declared a spec.

There is no intention to update the meta for a var when you declare an fdef spec.





[CLJ-1967] Enhanced namespaced map pprint support Created: 23/Jun/16  Updated: 22/Aug/16

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

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

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

 Description   

CLJ-1910 added namespaced map syntax for reader but did not include pprint support.

user=> (binding [clojure.pprint/*print-right-margin* 40]
         (pprint {:the.namespace/lajsdflkajsd 1 :the.namespace/aljsdfjasdf 2}))
#:the.namespace{:lajsdflkajsd 1,
                :aljsdfjasdf 2}
nil
user=> (binding [clojure.pprint/*print-right-margin* 40
                 *print-namespace-maps* false]
  (pprint {:the.namespace/lajsdflkajsd 1 :the.namespace/aljsdfjasdf 2}))
{:the.namespace/lajsdflkajsd 1,
 :the.namespace/aljsdfjasdf 2}

Patch: clj-1967.patch






[CLJ-2007] if-let* & when-let* with multiple bindings implementation Created: 21/Aug/16  Updated: 21/Aug/16

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

Type: Enhancement Priority: Major
Reporter: Ertuğrul Çetin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Patch: Code

 Description   

I think Clojure programs will be more elegant if we use let versions of if & when with multiple bindings.

;; if-let* imp.

(defmacro if-let*
  "Multiple binding version of if-let"
  ([bindings then]
   `(if-let* ~bindings ~then nil))
  ([bindings then else]
   (when (seq bindings)
     (assert-args
       (vector? bindings) "a vector for its binding"
       (even? (count bindings)) "exactly even forms in binding vector"))
   (if (seq bindings)
     `(if-let [~(first bindings) ~(second bindings)]
        (if-let* ~(vec (drop 2 bindings)) ~then ~else)
        ~(if-not (second bindings) else))
     then)))

;;Example if-let*

(if-let* [a 1
          b (+ a 1) ]
          b)

;;=> 2

(if-let* [a 1
          b (+ a 1)
          c false] ;;false or nil - does not matter
          b
          a)

;;=> 1

;; when-let* imp.

(defmacro when-let*
  "Multiple binding version of when-let"
  [bindings & body]
  (when (seq bindings)
    (assert-args
      (vector? bindings) "a vector for its binding"
      (even? (count bindings)) "exactly even forms in binding vector"))
  (if (seq bindings)
    `(when-let [~(first bindings) ~(second bindings)]
       (when-let* ~(vec (drop 2 bindings)) ~@body))
    `(do ~@body)))

;;Example when-let*

  (when-let* [a 1 
             b 2 
             c (+ a b)]
             (println "yeah!")
             c)
  ;;=>yeah!
  ;;=>3

  (when-let* [a 1 
             b nil 
             c 3]
             (println "damn! b is nil")
             a)
  ;;=>nil





[CLJ-2005] Type hint fails with direct linking disabled Created: 19/Aug/16  Updated: 20/Aug/16

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

Type: Defect Priority: Minor
Reporter: Viktor Magyari Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, directlinking, typehints

Attachments: Text File 0001-CLJ-2005-assoc-arglist-ret-tag-as-tag-in-constructed.patch    
Patch: Code
Approval: Triaged

 Description   

Minimal example, using 1.9.0-alpha11:

user=> (set! *warn-on-reflection* true)
true
user=> (defn foo ^String [^long x] "")
#'user/foo
user=> (.length (foo 10))
Reflection warning, (...) - reference to field length on java.lang.Object can't be resolved.
0

The warning is present only if direct linking is disabled.

Explanation:
this is another manifestation of CLJ-1533 – because of the lexical transformation the compiler is doing when routing the invoke through invokePrim, the arglists type hints are lost. This doesn't happen when DL is on because invokeStatic isn't compiled via a lexical transformation but through StaticInvokeExpr which properly tracks the original var's type hints

Patch: 0001-CLJ-2005-assoc-arglist-ret-tag-as-tag-in-constructed.patch



 Comments   
Comment by Nicola Mometto [ 19/Aug/16 5:24 PM ]

With DL on:

public static java.lang.Object invokeStatic();
    descriptor: ()Ljava/lang/Object;
    flags: ACC_PUBLIC, ACC_STATIC
    Code:
      stack=2, locals=0, args_size=0
         0: ldc2_w        #12                 // long 10l
         3: invokestatic  #18                 // Method test$foo.invokeStatic:(J)Ljava/lang/Object;
         6: checkcast     #20                 // class java/lang/String
         9: invokevirtual #24                 // Method java/lang/String.length:()I
        12: invokestatic  #30                 // Method java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
        15: areturn
      LineNumberTable:
        line 5: 0
        line 5: 9

with DL off:

public static java.lang.Object invokeStatic();
    descriptor: ()Ljava/lang/Object;
    flags: ACC_PUBLIC, ACC_STATIC
    Code:
      stack=3, locals=0, args_size=0
         0: getstatic     #15                 // Field const__0:Lclojure/lang/Var;
         3: invokevirtual #20                 // Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
         6: checkcast     #22                 // class clojure/lang/IFn$LO
         9: ldc2_w        #23                 // long 10l
        12: invokeinterface #28,  3           // InterfaceMethod clojure/lang/IFn$LO.invokePrim:(J)Ljava/lang/Object;
        17: ldc           #30                 // String length
        19: iconst_0
        20: invokestatic  #36                 // Method clojure/lang/Reflector.invokeNoArgInstanceMember:(Ljava/lang/Object;Ljava/lang/String;Z)Ljava/lang/Object;
        23: areturn
      LineNumberTable:
        line 5: 0
        line 5: 12
        line 5: 17
Comment by Nicola Mometto [ 19/Aug/16 5:43 PM ]

bytecode with DL off and current patch:

public static java.lang.Object invokeStatic();
    descriptor: ()Ljava/lang/Object;
    flags: ACC_PUBLIC, ACC_STATIC
    Code:
      stack=3, locals=0, args_size=0
         0: getstatic     #15                 // Field const__0:Lclojure/lang/Var;
         3: invokevirtual #20                 // Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
         6: checkcast     #22                 // class clojure/lang/IFn$LO
         9: ldc2_w        #23                 // long 10l
        12: invokeinterface #28,  3           // InterfaceMethod clojure/lang/IFn$LO.invokePrim:(J)Ljava/lang/Object;
        17: checkcast     #30                 // class java/lang/String
        20: invokevirtual #34                 // Method java/lang/String.length:()I
        23: invokestatic  #40                 // Method java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
        26: areturn
      LineNumberTable:
        line 5: 0
        line 5: 12
        line 5: 20




[CLJ-1149] Unhelpful error message from :use (and use function) when arguments are malformed Created: 17/Jan/13  Updated: 19/Aug/16

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

Type: Enhancement Priority: Major
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: errormsgs

Approval: Ok

 Description   

the following exception happens when you have something like this(bad):

(ns runtime.util-test
(:use [midje.sweet :reload-all]))

as opposed to any of these(correct):

(ns runtime.util-test
(:use midje.sweet :reload-all))

(ns runtime.util-test
(:use [midje.sweet] :reload-all))

and the exception is:
Exception in thread "main" java.lang.IllegalArgumentException: No value supplied for key: true
at clojure.lang.PersistentHashMap.create(PersistentHashMap.java:77)
at clojure.core$hash_map.doInvoke(core.clj:365)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:617)
at clojure.core$load_lib.doInvoke(core.clj:5352)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:619)
at clojure.core$load_libs.doInvoke(core.clj:5403)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:621)
at clojure.core$use.doInvoke(core.clj:5497)

Note that this is similar to the equally unhelpful message shown in http://dev.clojure.org/jira/browse/CLJ-1140 although that is a different root cause.

Probably best to enhance the `use` function to validate its arguments before trying to apply hash-map?



 Comments   
Comment by Gary Fredericks [ 26/May/13 3:17 PM ]

I believe this applies to require as well.

Comment by Alex Miller [ 19/Aug/16 2:25 PM ]

As of Clojure 1.9.0-alpha11, the spec for ns will catch this and fail at compile-time:

java.lang.IllegalArgumentException: Call to clojure.core/ns did not conform to spec:
In: [1] val: ((:use [midje.sweet :reload-all])) fails at: [:args] predicate: (cat :docstring (? string?) :attr-map (? map?) :clauses :clojure.core.specs/ns-clauses),  Extra input
:clojure.spec/args  (runtime.util-test (:use [midje.sweet :reload-all]))




[CLJ-1029] ns defmacro allows arbitrary execution of clojure.core fns Created: 23/Jul/12  Updated: 19/Aug/16  Resolved: 19/Aug/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2, Release 1.3, Release 1.4
Fix Version/s: Release 1.9

Type: Defect Priority: Minor
Reporter: Craig Brozefsky Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: error-reporting
Environment:

all


Attachments: File ns-patch.diff    
Patch: Code
Approval: Ok

 Description   

The form:

(ns foo (:print "I AM A ROBOT"))

will print "I AM A ROBOT"

This is because the defmacro takes the name of the first element of the reference, looks it up in the clojure.core namespace and invokes it on the rest of the args.

This is minor, but it does mean that an otherwise declarative form is not executing code.



 Comments   
Comment by Alan Malloy [ 25/Jul/12 4:37 PM ]

One apparent problem with this patch is that you throw an exception for :refer. You should add that, and make sure there aren't any others missing. Also, #{x y z} is better than (set [x y z]), and you should probably use pr-str rather than str, although I can't think of a case where it matters for the objects in question.

Comment by Andy Fingerhut [ 26/Jul/12 6:31 PM ]

A more minor detail of patch formatting – please attach your patch in git format. See the instructions under the section heading "Development" on this web page: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Craig Brozefsky [ 05/Aug/12 9:53 AM ]

git format-patch version of the diff, with the edits suggested by other maintainers.

Comment by Craig Brozefsky [ 05/Aug/12 10:00 AM ]

Alan: please note that :refer was not mentioned in the docstring for ns, or used in any of the unit tests for clojure.

Are you sure that it is an expected argument, or just an arrangement that happens to work under the current ns macro? The docstring for 'refer itself says to use :use in ns macros instead of calling refer.

I added "refer" to the set of accepted references all the same.

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

This is a case where better error checking would prevent this problem.

Comment by Alex Miller [ 19/Aug/16 2:24 PM ]

As of 1.9.0-alpha11 the spec for ns catches and rejects this invalid use of ns.





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

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

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

Reproducible on all platforms and all clojure versions.


Approval: Triaged

 Description   

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

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

It will report:

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

However, if is written:

(defn foo "bla")

error report makes more sense:

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


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

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

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

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

Comment by Alex Miller [ 19/Aug/16 2:21 PM ]

Due to the new specs for `defn` in Clojure 1.9.0-alpha11, both of these fail with more informative errors:

user=> (defn foo
  "This is docstring."
  (let [i 1]
    (+ i 1)))
CompilerException java.lang.IllegalArgumentException: Call to clojure.core/defn did not conform to spec:
In: [2] val: (let [i 1] (+ i 1)) fails spec: :clojure.core.specs/arg-list at: [:args :bs :arity-1 :args] predicate: vector?
In: [2 0] val: let fails spec: :clojure.core.specs/arg-list at: [:args :bs :arity-n :bodies :args] predicate: vector?
:clojure.spec/args  (foo "This is docstring." (let [i 1] (+ i 1)))
, compiling:(NO_SOURCE_PATH:5:1)

user=> (defn foo "bla")
CompilerException java.lang.IllegalArgumentException: Call to clojure.core/defn did not conform to spec:
val: () fails spec: :clojure.core.specs/defn-args at: [:args :bs] predicate: (alt :arity-1 :clojure.core.specs/args+body :arity-n (cat :bodies (+ (spec :clojure.core.specs/args+body)) :attr (? map?))),  Insufficient input
:clojure.spec/args  (foo "bla")
, compiling:(NO_SOURCE_PATH:9:1)

Any further changes in this will come through changes in either the specs or spec error reporting, so I am closing this.





[CLJ-1630] Destructuring allows multiple &-params Created: 31/Dec/14  Updated: 19/Aug/16

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

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

Attachments: Text File CLJ-1630-v2.patch     Text File no-multiple-rest-params-v1.patch    
Patch: Code and Test
Approval: Ok

 Description   

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



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

I see:

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

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

?

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

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

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

Comment by Alex Miller [ 19/Aug/16 2:16 PM ]

As of Clojure 1.9.0-alpha11, the `let` spec catches this and fails at compile time.





[CLJ-1870] Reloading a defmulti nukes metadata on the var Created: 22/Dec/15  Updated: 19/Aug/16  Resolved: 19/Aug/16

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: ft, metadata, multimethods

Attachments: Text File 0001-CLJ-1870-don-t-destroy-defmulti-metadata-on-reload.patch    
Patch: Code
Approval: Ok

 Description   

Reloading a defmulti expression destroys any existing (or new) metadata on that multimethod's var:

user=> (defmulti foo "docstring" :op)
#'user/foo
user=> (-> #'foo meta :doc)
"docstring"
user=> (defmulti foo "docstring" :op)
nil
user=> (-> #'foo meta :doc)
nil

This is highly problematic for tools.analyzer, since it relies on such metadata to convey information to the pass scheduler about pass dependencies.

This means that any code that uses core.async cannot be reloaded using `require :reload-all`, since it will cause tools.analyzer to reload and the passes to scheduled in a random order. See ASYNC-154 for one example.

Cause: defmulti has defonce semantics and the first def does not re-apply meta.

Approach: Re-apply meta before first def.

After patch:

user=> (defmulti foo "docstring" :op)
#'user/foo
user=> (-> #'foo meta :doc)
"docstring"
user=> (defmulti foo "docstring" :op)
nil
user=> (-> #'foo meta :doc)
"docstring"

Patch: 0001-CLJ-1870-don-t-destroy-defmulti-metadata-on-reload.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 23/Dec/15 10:16 AM ]

Related: CLJ-900 CLJ-1446

Comment by Alex Miller [ 23/Dec/15 10:42 AM ]

This patch doesn't compile for me: RuntimeException: Unable to resolve symbol: mm in this context, compiling:(clojure/core.clj:1679:17)

Also, please build the patch with more context - use -U10 with git format-patch to expand it.

Comment by Nicola Mometto [ 23/Dec/15 11:06 AM ]

Updated patch fixing the typo & using -U10

Comment by Alex Miller [ 23/Dec/15 11:17 AM ]

Now:

java.lang.RuntimeException: Too many arguments to def, compiling:(clojure/core.clj:3561:1)

Comment by Nicola Mometto [ 23/Dec/15 11:22 AM ]

whoops. Sorry for this, here's the updated (and working) patch

Comment by Alex Miller [ 19/Aug/16 10:58 AM ]

patch doesn't apply with new def spec, needs another look

Comment by Alex Miller [ 19/Aug/16 11:16 AM ]

I had an old version of the patch locally - nvm!





[CLJ-1914] Range realization has a race during concurrent execution Created: 14/Apr/16  Updated: 19/Aug/16  Resolved: 19/Aug/16

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

Type: Defect Priority: Critical
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: range

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

 Description   

When a range instance is enumerated via next concurrently, some threads may not see the entire range sequence. When this happens next will return nil prematurely.

(defn enumerate [r]
 (loop [rr r
        c []]
  (let [f (first rr)]
   (if f
    (recur (next rr) (conj c f))
    c))))

(defn demo [size threads]
 (let [r (range size)
       futures (doall (repeatedly threads #(future (enumerate r))))
       res (doall (map deref futures))]
  (if (apply = res)
   (println "Passed")
   (println "Failed, counts=" (map count res)))))

(demo 300 4)
Failed, counts= (300 64 300 64)

The demo above will reliably produce a failure every few executions like the one above.

The vast majority of the time, range is used either single-threaded or in a non-competing way where this scenario will never happen. This failure only occurs when two or more threads are enumerating rapidly through the same range instance.

Cause:

Each LongRange instance acts in several capacities - as a seq, a chunked seq, and a reducible, all of which represent independent enumeration strategies (multiple of which may be used by the user). LongRange holds 2 pieces of (volatile, non-synchronized) state related to chunking: _chunk and _chunkNext. That state is only updated in forceChunk(). forceChunk() uses the "racy single-check idiom" to tolerate multiple threads racing to create the chunk. That is, multiple threads may detect that the chunk has not been set (based on null _chunk) and BOTH threads will create the next chunk and write it. But both threads have good local values, compute the same next value, and set the same next values in the fields, so the order they finish is unimportant.

The problem here is that there are actually two fields, and they are set in the order _chunk then _chunkNext. Because the guard is based on _chunk, it's possible for a thread to think the chunk values have been set but _chunkNext hasn't yet been set.

Approach:

Moving the set for _chunkNext before the set for _chunk removes that narrow window of race opportunity.

Patch: clj-1914-3.patch

Thanks to Kyle Kingsbury for the initial reproducing case https://gist.github.com/aphyr/8746181beeac6a728a3aa018804d56f6



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

It's not necessary to synchronize here - just swapping these two lines should address the race I think: https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LongRange.java#L131-L132

Comment by Ghadi Shayban [ 14/Apr/16 11:22 PM ]

Good call. That's super subtle.

It appears all mutable assignment occurs in forceChunk() except for https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LongRange.java#L145 which should be OK (famous last words).

Comment by Stuart Halloway [ 27/Jul/16 3:27 PM ]

This ticket would benefit from an explanation of the failure mode caused by the race, plus an explanation of the subtle reasoning behind the fix.

Comment by Alex Miller [ 27/Jul/16 4:27 PM ]

Updated per request.





[CLJ-1423] Applying a var to an infinite arglist consumes all available memory Created: 15/May/14  Updated: 19/Aug/16  Resolved: 19/Aug/16

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

Type: Defect Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: performance

Attachments: Text File apply-var.patch     Text File clj-1423.patch    
Patch: Code and Test
Approval: Ok

 Description   

It is possible to apply a function to an infinite argument list: for example, (apply distinct? (repeat 1)) immediately returns false, after realizing just a few elements of the infinite sequence (repeat 1). However, (apply #'distinct? (repeat 1)) attempts to realize all of (repeat 1) into memory at once.

This happens because Var.applyTo delegates to AFn.applyToHelper to decide which arity of Var.invoke to dispatch to; but AFn doesn't expect infinite arglists (mostly those use RestFn). So it uses RT.seqToArray, which doesn't work well in this case.

Instead, Var.applyTo(args) can just dispatch to deref().applyTo(args), and let the function being stored figure out what to do with the arglist.

I've changed Var.applyTo to do this, and added a test (which fails before my patch is applied, and passes afterwards).

Patch: clj-1423.patch

Screened by: Alex Miller



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

I also did a quick perf test on this:

(dotimes [x 20] (time (dotimes [y 10000] (apply #'+ (range 100)))))

which showed times ~82 ms per rep before the patch and ~10 ms per rep after the patch (on par with applying the function directly).

I added a new patch that squashes the commits, adds the ticket number to the commit message, attribution was retained.





[CLJ-1744] Unused destructured local not cleared, causes memory leak Created: 03/Jun/15  Updated: 19/Aug/16  Resolved: 19/Aug/16

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 17
Labels: compiler, destructuring, locals-clearing, memory

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

 Description   

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

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

Here's a macroexpansion that exposes this issue:

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

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

Approach: Track whether local bindings are used. After evaluating the binding expression, if the local binding is not used and can be cleared, then pop the result rather than storing it.

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

Screened by: Alex Miller



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

Nice =)

Comment by James Henderson [ 18/Nov/15 6:12 AM ]

FYI - we've hit this as a memory leak in our production system:

(defn write-response! [{:keys [products merchant-id] :as search-result} writer response-type]
  ;; not using `search-result` throughout this fn - kept in to document intent
  ;; hangs on to `products`, a large lazy-seq, until it's completely consumed, causes memory leak

  ...
  (case response-type
    "application/edn" (print-method products writer)
    ...))
(defn write-response! [{:keys [products merchant-id]} writer response-type]
  ;; fine, releases earlier elements in products as it flies through the response

  ...
  (case response-type
    "application/edn" (print-method products writer)
    ...))

The work-around in our case is easy enough - removing the unused symbol - but, given we assumed including an unused symbol would be a no-op, it did take us a while to find!

Cheers,

James

Comment by Nicola Mometto [ 15/Dec/15 11:47 AM ]

While investigating an unrelated memory leak on core.async (ASYNC-138) I discovered that this bug also affects code inside a `go` macro, since it often emits unreachable bindings

clojure.core.async> (macroexpand '(go (let [test (range 1e10)]
                                        (str test (<! (chan))))))
(let*
 [c__15123__auto__ (clojure.core.async/chan 1) captured-bindings__15124__auto__ (clojure.lang.Var/getThreadBindingFrame)]
 (clojure.core.async.impl.dispatch/run
  (fn*
   []
   (clojure.core/let
    [f__15125__auto__
     (clojure.core/fn
      state-machine__14945__auto__
      ([] (clojure.core.async.impl.ioc-macros/aset-all! (java.util.concurrent.atomic.AtomicReferenceArray. 9) 0 state-machine__14945__auto__ 1 1))
      ([state_17532]
       (clojure.core/let
        [old-frame__14946__auto__
         (clojure.lang.Var/getThreadBindingFrame)
         ret-value__14947__auto__
         (try
          (clojure.lang.Var/resetThreadBindingFrame (clojure.core.async.impl.ioc-macros/aget-object state_17532 3))
          (clojure.core/loop
           []
           (clojure.core/let
            [result__14948__auto__
             (clojure.core/case
              (clojure.core/int (clojure.core.async.impl.ioc-macros/aget-object state_17532 1))
              1
              (clojure.core/let
               [inst_17525 (clojure.core.async.impl.ioc-macros/aget-object state_17532 7) inst_17525 (range 1.0E10) test inst_17525 inst_17526 str test inst_17525 inst_17527 (chan) state_17532 (clojure.core.async.impl.ioc-macros/aset-all! state_17532 7 inst_17525 8 inst_17526)]
               (clojure.core.async.impl.ioc-macros/take! state_17532 2 inst_17527))
              2
              (clojure.core/let
               [inst_17525 (clojure.core.async.impl.ioc-macros/aget-object state_17532 7) inst_17526 (clojure.core.async.impl.ioc-macros/aget-object state_17532 8) inst_17529 (clojure.core.async.impl.ioc-macros/aget-object state_17532 2) inst_17530 (inst_17526 inst_17525 inst_17529)]
               (clojure.core.async.impl.ioc-macros/return-chan state_17532 inst_17530)))]
            (if (clojure.core/identical? result__14948__auto__ :recur) (recur) result__14948__auto__)))
          (catch java.lang.Throwable ex__14949__auto__ (clojure.core.async.impl.ioc-macros/aset-all! state_17532 clojure.core.async.impl.ioc-macros/CURRENT-EXCEPTION ex__14949__auto__) (clojure.core.async.impl.ioc-macros/process-exception state_17532) :recur)
          (finally (clojure.lang.Var/resetThreadBindingFrame old-frame__14946__auto__)))]
        (if (clojure.core/identical? ret-value__14947__auto__ :recur) (recur state_17532) ret-value__14947__auto__))))
     state__15126__auto__
     (clojure.core/-> (f__15125__auto__) (clojure.core.async.impl.ioc-macros/aset-all! clojure.core.async.impl.ioc-macros/USER-START-IDX c__15123__auto__ clojure.core.async.impl.ioc-macros/BINDINGS-IDX captured-bindings__15124__auto__))]
    (clojure.core.async.impl.ioc-macros/run-state-machine-wrapped state__15126__auto__))))
 c__15123__auto__)




[CLJ-1993] Print flag to suppress namespace map syntax Created: 28/Jul/16  Updated: 19/Aug/16  Resolved: 16/Aug/16

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

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

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

 Description   

Add new print flag *print-namespace-maps* to optionally suppress namespace map syntax. Default flag to false.

Set flag to true as part of the standard REPL bindings. This allows repl users to (set! *print-namespace-maps* false) if desired.

Patch: clj-1993-2.patch



 Comments   
Comment by Rich Hickey [ 16/Aug/16 7:33 AM ]

the plan/approach should be somewhere in the description and not just the code please. Also, I wonder if the default should be false other than at the REPL?

Comment by Alex Miller [ 16/Aug/16 6:53 PM ]

Committed for next alpha





[CLJ-1406] Libs are blindly added into loaded-libs even if an error occurs during loading Created: 17/Apr/14  Updated: 19/Aug/16

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

Type: Defect Priority: Major
Reporter: OHTA Shogo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-modify-clojure.core-load-lib-so-that-it-removes-the-.patch    
Patch: Code
Approval: Vetted

 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 OHTA Shogo [ 17/Apr/14 9:21 AM ]

To do so, I think we need to revert CLJ-1116.





[CLJ-1242] = on sorted collections with different key types incorrectly throws Created: 31/Jul/13  Updated: 19/Aug/16

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: collections

Attachments: Text File 0001-fix-for-CLJ-1242-tests.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Comparing a sorted-set with numbers to a set with keywords is not symmetric:

user=> (= #{:a} (sorted-set 1))
false
user=> (= (sorted-set 1) #{:a})
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword  clojure.lang.Keyword.compareTo (Keyword.java:109)

The latter case should return false instead of throwing.

Cause: APersistentMap.equiv() and APersistentSet.equiv() do not expect this exception be thrown from the containsKey()/contains() check. It would probably be best for PersistentTreeMap and PersistentTreeMap to implement equiv() and handle that possibility appropriately. Should also consider similar changes in equals() if necessary.

See also: CLJ-1983 (downstream example with clojure.data/diff)



 Comments   
Comment by OHTA Shogo [ 31/Jul/13 8:02 PM ]

PersistentVector also has the same problem.

user=> (compare [1] [:a])
java.lang.ClassCastException: clojure.lang.Keyword cannot be cast to java.lang.Number

The cause of this problem is that Util.compare() casts the second argument
to Number without checking its type when the first argument is a Number.

Comment by OHTA Shogo [ 31/Jul/13 8:26 PM ]

Umm, my brain was not working right.
Util.compare() should raise an Exception when the arguments' type are different.

Comment by François Rey [ 02/May/15 4:44 PM ]

Upvoting.
Here's a instance of this bug in codox:
https://github.com/weavejester/codox/issues/91

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

The behavior of get is consistent with Java collections, so I think changing that expectation should be considered a feature request and not a bug.

The fix for equals should be informed by the approach taken in the JDK, where the implementation of equals (not get) has exception catchers.

Comment by Alex Miller [ 21/Jan/16 10:33 AM ]

I re-focused this ticket on just the equality aspect. The other request regarding `get` with a value of a different type is consistent with Java behavior and should be considered "as designed" - a separate enhancement ticket could be considered for that one.

user=> (def s (java.util.TreeSet.))
#'user/s
user=> (.add s 1)
true
user=> (.contains s "a")
ClassCastException java.lang.Long cannot be cast to java.lang.String  java.lang.String.compareTo (String.java:108)
Comment by Alex Miller [ 19/Jul/16 2:00 PM ]

oops, sorry for the close/reopen.

Comment by Rich Hickey [ 19/Aug/16 10:47 AM ]

Stu - link for "The fix for equals should be informed by the approach taken in the JDK, where the implementation of equals (not get) has exception catchers." ?





[CLJ-1879] reduce-kv on a PHMs doesn't consistently execute the intended fastpath Created: 09/Jan/16  Updated: 19/Aug/16

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

Type: Defect Priority: Major
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File CLJ-1879.patch    
Approval: Vetted

 Description   

https://github.com/clojure/clojure/blob/010864f/src/clj/clojure/core.clj#L6553-L6562

Because PHMs implement clojure.lang.IKVReduce and IPersistentMap, they have nondeterministic dispatch through the protocol that backs reduce-kv (clojure.core.protocols/IKVReduce).

A potential way to solve this is to add an instance check for clojure.lang.IKVReduce inside `reduce-kv` (This is similar to how reduce checks for IReduceInit)



 Comments   
Comment by Nicola Mometto [ 11/Jan/16 9:23 AM ]

CLJ-1807 offers a generic solution for this class of problems





[CLJ-787] transient blows up when passed a vector created by subvec Created: 03/May/11  Updated: 19/Aug/16

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

Type: Defect Priority: Major
Reporter: Alexander Redington Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File CLJ-787-p1.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Subvectors created with subvec from a PersistentVector cannot be made transient:

user=> (transient (subvec [1 2 3 4] 2))
ClassCastException clojure.lang.APersistentVector$SubVector cannot be cast to clojure.lang.IEditableCollection  clojure.core/transient (core.clj:2864)

Cause: APersistentVector$SubVector does not implement IEditableCollection

Patch: CLJ-787-p1.patch

Approach: Create a TransientSubVector based on an underlying TransientVector.

Two assumptions:

  • It's okay for TransientSubVector to delegate the ensureEditable functionality to the underlying TransientVector (sometimes explicitly, sometimes implicitly) - calling ensureEditable explicitly also requires that the field for the underlying vector be the concrete TransientVector type rather than the ITransientVector interface.
  • When an operation that would throw an exception on a PersistentVector happens from the wrong thread (or after persistent!), we throw that exception rather than the IllegalAccessError that transients throw when accessed inappropriately.


 Comments   
Comment by Stuart Sierra [ 31/May/11 9:28 AM ]

Confirmed. APersistentVector$SubVector does not implement IEditableCollection.

The current implementation of TransientVector depends on implementation details of PersistentVector, so it is not a trivial fix. The simplest fix might be to implement IEditableCollection.asTransient in SubVector by creating a new PersistentVector, but I do not know the performance implications.

Comment by Gary Fredericks [ 25/May/13 8:11 PM ]

We could get the same performance characteristics as SubVector by creating a TransientSubVector based on an underlying TransientVector, right?

Preparing a patch to that effect.

Comment by Gary Fredericks [ 25/May/13 10:58 PM ]

Text from the commit msg:

Made two assumptions:

  • It's okay for TransientSubVector to delegate the ensureEditable
    functionality to the underlying TransientVector (sometimes
    explicitely, sometimes implicitely) – calling ensureEditable
    explicitely also requires that the field for the underlying vector
    be the concrete TransientVector type rather than the
    ITransientVector interface.
  • When an operation that would throw an exception on a
    PersistentVector happens from the wrong thread (or after
    persistent!), we throw that exception rather than the
    IllegalAccessError that transients throw when accessed
    inappropriately.
Comment by Alex Miller [ 11/Oct/13 4:17 PM ]

I think there are some assumptions being made in this patch about the class structure here that do not hold. The structure is, admittedly, quite twisty.

A counter-example that highlights one of a few subtypes of APersistentVector that are not PersistentVector (like MapEntry):

user=> (transient (subvec (first {:a 1}) 0 1))
ClassCastException clojure.lang.MapEntry cannot be cast to clojure.lang.IEditableCollection  clojure.lang.APersistentVector$TransientSubVector.<init> (APersistentVector.java:592)

PersistentVector.SubVector expects to work on anything that implements IPersistentVector. Note that this includes concrete types such as MapEntry and LazilyPersistentVector, but could also be any user-implemented type IPersistentVector type too. TransientSubVector is making the assumption that the IPersistentVector in a SubVector question is also an IEditableCollection (that can be converted to be transient). Note that while PersistentVector implements TransientVector (and IEditableCollection), APersistentVector does not. To really implement this in tandem with SubVector, I think you would need to guarantee that IPersistentVector extended IEditableCollection and I don't think that's something we want to do.

I don't see an easy solution. Any time I see all these modifiers (Transient, Sub, etc) being created in different combinations, it is a clear sign that independent kinds of functionality are being remixed into single inheritance OO trees. You can see the same thing in most collection libraries (even Java's - need a ConcurrentIdentitySortedMap? too bad!).

Needs more thought.

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

Patch CLJ-787-p1.patch no longer applies cleanly to latest master, but it is only because of some new tests added to the transients.clj file since the patch was created, so it is trivial to update in that sense. Not updating it now due to other more significant issues with the patch described above.

Comment by Alex Miller [ 17/Jan/14 10:19 AM ]

No good solution to consider right now, removing from 1.6.





[CLJ-1872] empty? is broken for transient collections Created: 26/Dec/15  Updated: 19/Aug/16

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

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-700] contains? broken for transient collections Created: 01/Jan/11  Updated: 19/Aug/16

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

Type: Defect Priority: Critical
Reporter: Herwig Hochleitner Assignee: Rich Hickey
Resolution: Unresolved Votes: 20
Labels: transient

Attachments: Java Source File 0001-Refactor-of-some-of-the-clojure-.java-code-to-fix-CL.patch     File clj-700-7.diff     File clj-700-8.diff     Text File clj-700-9.patch     File clj-700.diff     Text File clj-700-patch4.txt     Text File clj-700-patch6.txt     Text File clj-700-rt.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Behavior with Clojure 1.6.0:

user=> (contains? (transient {:x "fine"}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentArrayMap$TransientArrayMap  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient (hash-map :x "fine")) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashMap$TransientHashMap  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient [1 2 3]) 0)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentVector$TransientVector  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient #{:x}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashSet$TransientHashSet  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (:x (transient #{:x}))
nil
;; expected: :x

user=> (get (transient #{:x}) :x)
nil
;; expected: :x

Cause: This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

Approach: Expand the types that RT.getFrom(), RT.contains(), and RT.find() can handle to cover the additional transient interfaces.

Alternative: Other older patches (prob best exemplified by clj-700-8.diff) restructure the collections type hierarchy. That is a much bigger change than the one taken here but is perhaps a better long-term path. That patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()). With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience.

Screening Notes

  • the extra conditions in RT add branches to some key functions. get already has a getFrom optimization, but there is no similar containsFrom or findFrom. Is it worth measuring the possible impact of these?
  • I believe the interface refactoring approach (not taken here) is worth separate consideration as an enhancement. If this is done, I think leveraging valAt would be simpler, e.g. allowing HashMap and ArrayMap to share code
  • it is not evident (to me anyway) why some API fns consume ILookup and others do not, among e.g. contains?, get, and find. Possible doc enhancement?
  • there is test code already in place (data_structures.clj) that could easily be expanded to cover transients. It would be nice to do this, or better yet get some test.check tests in place

Patch: clj-700-9.patch



 Comments   
Comment by Herwig Hochleitner [ 01/Jan/11 8:01 PM ]

the same is also true for TransientVectors

{{(contains? (transient [1 2 3]) 0)}}

false

Comment by Herwig Hochleitner [ 01/Jan/11 8:25 PM ]

As expected, TransientSets have the same issue; plus an additional, probably related one.

(:x (transient #{:x}))

nil

(get (transient #{:x}) :x)

nil

Comment by Alexander Redington [ 07/Jan/11 2:07 PM ]

This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Comment by Stuart Halloway [ 28/Jan/11 10:35 AM ]

Rich: Patch doesn't currently apply, but I would like to get your take on approach here. In particular:

  1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?
  2. what are good names for the interfaces introduced here?
Comment by Alexander Redington [ 25/Mar/11 7:44 AM ]

Rebased the patch off the latest pull of master as of 3/25/2011, it should apply cleanly now.

Comment by Stuart Sierra [ 17/Feb/12 2:59 PM ]

Latest patch does not apply as of f5bcf647

Comment by Andy Fingerhut [ 17/Feb/12 5:59 PM ]

clj-700-patch2.txt does patch cleanly to latest Clojure head as of a few mins ago. No changes to patch except in context around changed lines.

Comment by Andy Fingerhut [ 07/Mar/12 3:23 AM ]

Sigh. Git patches applied via 'git am' are fragile beasts indeed. Look at them the wrong way and they fail to apply.

clj-700-patch3.txt applies cleanly to latest master as of Mar 7, 2012, but not if you use this command:

git am -s < clj-700-patch3.txt

I am pretty sure this is because of DOS CR/LF line endings in the file src/jvm/clojure/lang/Associative.java. The patch does apply cleanly if you use this command:

git am --keep-cr -s < clj-700-patch3.txt

Comment by Andy Fingerhut [ 23/Mar/12 6:34 PM ]

This ticket was changed to Incomplete and waiting on Rich when Stuart Halloway asked for feedback on the approach on 28/Jan/2011. Stuart Sierra changed it to not waiting on Rich on 17/Feb/2012 when he noted the patch didn't apply cleanly. Latest patch clj-700-patch3.txt does apply cleanly, but doesn't change the approach used since the time Stuart Halloway's concern was raised. Should it be marked as waiting on Rich again? Something else?

Comment by Stuart Halloway [ 08/Jun/12 12:44 PM ]

Patch 4 incorporates patch 3, and brings it up to date on hashing (i.e. uses hasheq).

Comment by Andy Fingerhut [ 08/Jun/12 12:52 PM ]

Removed clj-700-patch3.txt in favor of Stuart Halloway's improved clj-700-patch4.txt dated June 8, 2012.

Comment by Andy Fingerhut [ 18/Jun/12 3:06 PM ]

clj-700-patch5.txt dated June 18, 2012 is the same as Stuart Halloway's clj-700-patch4.txt, except for context lines that have changed in Clojure master since Stuart's patch was created. clj-700-patch4.txt no longer applies cleanly.

Comment by Andy Fingerhut [ 19/Aug/12 4:47 AM ]

Adding clj-700-patch6.txt, which is identical to Stuart Halloway's clj-700-patch4.txt, except that it applies cleanly to latest master as of Aug 19, 2012. Note that as described above, you must use the --keep-cr option to 'git am' when applying this patch for it to succeed. Removing clj-700-patch5.txt, since it no longer applies cleanly.

Comment by Stuart Sierra [ 24/Aug/12 1:08 PM ]

Patch fails as of commit 1c8eb16a14ce5daefef1df68d2f6b1f143003140

Comment by Andy Fingerhut [ 24/Aug/12 1:53 PM ]

Which patch did you try, and what command did you use? I tried applying clj-700-patch6.txt to the same commit, using the following command, and it applied, albeit with the warning messages shown:

% git am --keep-cr -s < clj-700-patch6.txt
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/jafinger/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq

Note the --keep-cr option, which is necessary for this patch to succeed. It is recommended in the "Screening Tickets" section of the JIRA workflow wiki page here: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Andy Fingerhut [ 28/Aug/12 5:48 PM ]

Presumptuously changing Approval from Incomplete back to None, since the latest patch does apply cleanly if the --keep-cr option is used. It was in Screened state recently, but I'm not so presumptuous as to change it to Screened

Comment by Alex Miller [ 19/Aug/13 12:26 PM ]

I think through a series of different hands on this ticket it got knocked way back in the list. Re-marking vetted as it's previously been all the way up through screening. Should also keep an eye on CLJ-787 as it may have some collisions with this one.

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

clj-700-7.diff is identical to clj-700-patch6.txt, except it applies cleanly to latest master. Only some lines of context in a test file have changed.

When I say "applies cleanly", I mean that there is one warning when using the proper "git am" command from the dev wiki page. This is because one line replaced in Associative.java has a CR/LF at the end of the line, because all lines in that file do.

Comment by Herwig Hochleitner [ 17/Feb/14 9:54 AM ]

Since clojure 1.5, contains? throws an IllegalArgumentException on transients.
In 1.6.0-beta1, transients are no longer marked as alpha.

Does this mean, that we won't be able to distinguish between a nil value and no value on a transient?

Comment by Stuart Halloway [ 27/Jun/14 10:20 AM ]

Request for someone to (1) update patch to apply cleanly, and (2) summarize approach so I don't have to read through the comment history.

Comment by Andy Fingerhut [ 27/Jun/14 11:02 AM ]

The latest patch is clj-700-7.diff dated Nov 8, 2013. I believe it is impossible to create a patch that applies any more cleanly using git for source files that have carriage returns in them, which at least one modified source file does. Here is the command I used on latest Clojure master as of today (Jun 27 2014), which is the same as that of March 25 2014:

% git am -s --keep-cr --ignore-whitespace < ~/clj/patches/clj-700-7.diff 
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/admin/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq

If you want a patch that doesn't have the 'trailing whitespace' warning in it, I think someone would have to commit a change that removed the carriage returns from file Associative.java. If you want such a patch, let me know and we can remove all of them from every source file and be done with this annoyance.

Comment by Andy Fingerhut [ 27/Jun/14 11:19 AM ]

Updated description to contain a copy of only those comments that seemed 'interesting'. Most comments have simply been "attached an updated patch that applies cleanly", or "changed the state of this ticket for reason X".

Comment by Alex Miller [ 27/Jun/14 1:19 PM ]

Looks like Andy did as requested, moving back to Screenable.

Comment by Andy Fingerhut [ 29/Aug/14 4:27 PM ]

Patch clj-700-7.diff dated Nov 8 2013 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.

Comment by Andy Fingerhut [ 01/Sep/14 3:59 AM ]

Patch clj-700-8.diff dated Sep 1 2014 is identical to clj-700-7.diff, except that it applies "cleanly" to latest master, by which I mean it applies as cleanly as I think it is possible to apply for a git patch to a file with carriage return/line feed line endings, as one of the modified files still does.

Comment by Alex Miller [ 17/Dec/14 3:12 PM ]

Added new patch with alternate approach that just makes RT know about transients instead of refactoring the class hierarchy.

clj-700-rt.patch

In some ways I think the class hierarchy refactoring is due, but I'm not totally on board with all the changes in those patches and it has impacts on collections outside Clojure itself that are hard to reason about.

Comment by Rich Hickey [ 31/Jul/15 11:35 AM ]

I'd like to look at the type hierarchy myself

Comment by Tim McCormack [ 14/Dec/15 1:09 PM ]

Workaround: Use ternary get with an acceptable sentinal value:

(get (transient {:x 5}) :y :no)
:no




[CLJ-1771] Support for multiple key(s)-value pairs in assoc-in Created: 29/Jun/15  Updated: 19/Aug/16

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

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

All


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

 Description   

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

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


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

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

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





[CLJ-2004] s/multispec doesn't include :retag in `s/form` Created: 18/Aug/16  Updated: 18/Aug/16

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

Type: Defect Priority: Major
Reporter: Allen Rohner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Vetted

 Description   

`s/form` on a multispec doesn't include the :retag, so it's impossible to recover that information from an existing spec. This is important for tooling that interacts with specs.



 Comments   
Comment by Alex Miller [ 18/Aug/16 6:18 PM ]

Yeah, this is one of many known form issues.





[CLJ-1936] spec generative testing should be optional Created: 28/May/16  Updated: 18/Aug/16

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

Type: Defect Priority: Major
Reporter: Allen Rohner Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: spec, test


 Description   

When using spec on side-effecting functions, I'd like to configure instrumentation (i.e. checking args and return value conform) separately from generative testing.

Proof that generative testing happens without calling clojure.test:

(defn foo [fnn] (fnn 42))
(s/fdef foo :args (s/cat :f (s/fspec :args (s/cat :i integer?)
                                     :ret integer?)))

(foo #(do (println %) (when (even? %) 42)))

=> (foo #(do (println %) (when (even? %) 42)))
42

(s/instrument 'foo)

=> (foo #(do (println %) (when (even? %) 42)))
0
-1
0
0
0
-1
0
0
-1
0
-1

ExceptionInfo Call to #'repl/foo did not conform to spec:
In: [0] val: nil fails at: [:args :f :ret] predicate: integer?
:clojure.spec/args  (#object[repl$eval67350$fn__67351 0x7bc3811c "repl$eval67350$fn__67351@7bc3811c"])
  clojure.core/ex-info (core.clj:4617)


 Comments   
Comment by Zach Oakes [ 28/May/16 9:01 PM ]

I think it would make sense to add something like core.typed's ^:no-check for this. For example:

(s/fdef ^:no-check foo :args (s/cat :f (s/fspec :args (s/cat :i integer?) :ret integer?)))

As a stopgap measure, I made a boot task that has a copy of clojure.spec.test/run-all-tests and modifies it to ignore vars with that metadata. That means I have to add it to the metadata in defn rather than fdef but it still seems to work.

Comment by Sean Corfield [ 01/Jun/16 9:32 PM ]

Yes, there are definitely situations where I would want argument / return spec checking on calls during dev/test but absolutely need the function excluded from generative testing.

Comment by Sean Corfield [ 01/Jun/16 9:38 PM ]

If you don't have test.check on your classpath, the call to s/instrument succeeds but then attempting to call foo fails with:

boot.user=> (defn foo [fnn] (fnn 42))
#'boot.user/foo
boot.user=> (s/fdef foo :args (s/cat :f (s/fspec :args (s/cat :i integer?)
       #_=>                                      :ret integer?)))
boot.user/foo
boot.user=> 

boot.user=> (foo #(do (println %) (when (even? %) 42)))
42
42
boot.user=> (s/instrument 'foo)
#'boot.user/foo
boot.user=> (foo #(do (println %) (when (even? %) 42)))

java.io.FileNotFoundException: Could not locate clojure/test/check/generators__init.class or clojure/test/check/generators.clj on classpath.

That is certainly unexpected and not very friendly!

Comment by Alex Miller [ 28/Jun/16 10:05 PM ]

There are new options in instrument as of 1.9.0-alpha8 that allow you to stub/mock functions. Those are one potential answer to this and maybe the recommended one, although I haven't used them enough to say that for sure.

Comment by Alex Miller [ 05/Jul/16 10:52 AM ]

See also CLJ-1976.

Comment by Sean Corfield [ 18/Aug/16 4:08 PM ]

Given that recent Alpha builds no longer check :ret or :fn with instrumentation, this issue seems to be resolved Alex Miller?

Comment by Allen Rohner [ 18/Aug/16 4:22 PM ]

fspec still requires generative testing.

Comment by Sean Corfield [ 18/Aug/16 4:24 PM ]

Ah, OK, I thought that had also rolled back to just :args testing at this point (I hadn't retested this since we have test.check as dev/test dependency now anyway).





[CLJ-1591] Symbol not being bound in namespace when name clashes with clojure.core Created: 14/Nov/14  Updated: 18/Aug/16

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

Type: Defect Priority: Trivial
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: None


 Description   

The following code fails (both in 1.6 and latest 1.7-alpha4):

user=> (ns foo)
nil
foo=>  (def inc inc)
WARNING: inc already refers to: #'clojure.core/inc in namespace: foo, being replaced by: #'foo/inc
#'foo/inc

;; Note inc is unbound at this point, which causes the exception below
foo=> inc
#<Unbound Unbound: #'foo/inc>
foo=> (ns bar)
nil
bar=> (require ['foo :refer ['inc]])
WARNING: inc already refers to: #'clojure.core/inc in namespace: bar, being replaced by: #'foo/inc
nil
bar=> (inc 8)

IllegalStateException Attempting to call unbound fn: #'foo/inc  clojure.lang.Var$Unbound.throwArity (Var.java:43)

Further investigation shows that foo/inc is unbound:

foo/inc
=> #<Unbound Unbound: #'foo/inc>

Further investigation also shows that replacing the (def inc inc) with almost anything else, e.g. (def inc dec), (def inc clojure.core/inc), or (def inc (fn [n] (+ n 1))), causes no exception (but the warnings remain).

I would expect:
a) foo/inc should be bound and have the same value as clojure.core/inc
b) No error when requiring foo/inc
c) bar/inc should be bound to foo/inc



 Comments   
Comment by Nicola Mometto [ 14/Nov/14 10:04 PM ]

The second error should be expected, the right syntax should be (require ['foo :refer ['inc]]) (note the leading quote before inc)

Comment by Mike Anderson [ 14/Nov/14 10:20 PM ]

Thanks for the catch Nicola - I've edited the description. Still get the same error however (just with a slightly different message)

Comment by Alex Miller [ 14/Nov/14 10:22 PM ]

See comment...

Comment by Mike Anderson [ 14/Nov/14 10:24 PM ]

@Alex what comment? Note that the error still occurs even with the right syntax....

Comment by Mike Anderson [ 14/Nov/14 10:26 PM ]

Appears to have been closed prematurely

Comment by Alex Miller [ 14/Nov/14 10:39 PM ]

I can't reproduce with the correct syntax:

Clojure 1.7.0-master-SNAPSHOT
user=> (ns foo)
nil
foo=> (def inc inc)
WARNING: inc already refers to: #'clojure.core/inc in namespace: foo, being replaced by: #'foo/inc
#'foo/inc
foo=> (ns bar)
nil
bar=> (require ['foo :refer ['inc]])
WARNING: inc already refers to: #'clojure.core/inc in namespace: bar, being replaced by: #'foo/inc
nil
Comment by Mike Anderson [ 14/Nov/14 10:55 PM ]

The problem is that the var is still unbound and causes e.g. the following error:

=> (foo/inc 8)
IllegalStateException Attempting to call unbound fn: #'foo/inc clojure.lang.Var$Unbound.throwArity (Var.java:43)

I don't think that should be expected - or am I missing something?

Comment by Alex Miller [ 14/Nov/14 10:57 PM ]

Ah, will take a look. But not right now.

Comment by Andy Fingerhut [ 15/Nov/14 1:09 PM ]

Updated the description with a few more details. The exception goes away if you do (def inc (fn [n] (+ n 1))) instead of (def inc inc), for example. The warnings remain.

Comment by Tom Crayford [ 20/Nov/14 11:07 AM ]

Unsure if this is the same issue (I think it might be?), but I reproduced the exact same error message with AOT compilation involved:

reproduced in this git repository: https://github.com/yeller/compiler_update_not_referenced_bug

clone it, run `lein do clean, uberjar, test`, and that error message will show up every time for me

Comment by Andy Fingerhut [ 20/Nov/14 5:43 PM ]

Mike, I think replacing (def inc inc) in your example with (def inc clojure.core/inc) should be considered as a reasonable workaround for this issue, unless you have some use case where you need to def inc to something that is not in clojure.core (and if so, why?)

The reason (def inc inc) behaves this way is, if not absolutely necessary, at least commonly used in Clojure programs to define recursive functions, e.g. (defn fib [n] (if (<= n 1) 1 (+ (fib (dec n)) (fib (- n 2))))), so that the occurrences of fib in the body are resolved to the fib being defined.

Comment by Alex Miller [ 22/Nov/14 9:05 AM ]

Moving to 1.7 until I can look at this more deeply.

Comment by Mike Anderson [ 23/Nov/14 6:08 PM ]

Andy - yes the workaround is fine for me right now.

I don't think this is an urgent issue but it may be exposing a subtle complexity regarding assumptions about the state of the namespace at different times. Perhaps the semantics should be something like:

  • The def statement itself should be run before the var is interned. e.g. (def inc (inc 5)) should result in (def inc 6)
  • Anything complied / deferred to run after completion of the def statement should use the new var (i.e. the new var should be referenced by fns, lazy sequences etc.)
Comment by Andy Fingerhut [ 23/Nov/14 6:36 PM ]

I'm not sure what your proposal means in a case like this:

(def inc (fn [x] (inc x)))

Is the second inc to be interpreted/resolved before or after the new inc is created? Because it is (fn ...) it should be the after-behavior? What else besides fn should cause the after-behavior, rather than the before-behavior?

Even more fun (not saying that people often write code like this, but the compiler can handle it today):

(def inc (if (> (inc y) 5)
           (fn [x] (inc x))
           (fn [x] (dec x))))

I think the current compiler behavior of 'in the body of a def, the def'd symbol always refers to the new var, not any earlier def'd vars' is fairly straightforward to explain.

Comment by Tom Crayford [ 23/Nov/14 9:15 PM ]

Should I file the AOT issue reproduced in that thing as a new issue?

Comment by Andy Fingerhut [ 24/Nov/14 5:16 PM ]

Tom: Alex Miller or another screener would be best to say whether the AOT issue should be a separate ticket, but my best guess would be "go for it". I tried to look at the link you gave but it seems not to point to anything. Could you double-check that link?

Comment by Tom Crayford [ 24/Nov/14 6:48 PM ]

Andy,

Great. I'll write one up tomorrow sometime. I accidentally left that repo as private, should be visible now.

Comment by Andy Fingerhut [ 24/Nov/14 8:11 PM ]

This comment is really most relevant for ticket CLJ-1604, where it has been copied:

Tom, looked at your project. Thanks for that. It appears not to have anything like (def inc inc) in it. It throws exception during test step of 'lein do clean, uberjar, test' consistently for me, too, but compiles with only warnings and passes tests with 'lein do clean, test'. I have more test results showing in which Clojure versions these results change. To summarize, the changes to Clojure that appear to make the biggest difference in the results are below (these should be added to the new ticket you create – you are welcome to do so):

Clojure 1.6.0, 1.7.0-alpha1, and later changes up through the commit with description "CLJ-1378: Allows FnExpr to override its reported class with a type hint": No errors or warnings for either lein command above.

Next commit with description "Add clojure.core/update, like update-in but takes a single key" that adds clojure.core/update: 'lein do clean, test' is fine, but 'lein do clean, uberjar' throws exception during compilation, probably due to CLJ-1241.

Next commit with description "fix CLJ-1241": 'lein do clean, test' and 'lein do clean, uberjar' give warnings about clojure.core/update, but no errors or exceptions. 'lein do clean, uberjar, test' throws exception during test step that is same as the one I see with Clojure 1.7.0-alpha4. Debug prints of values of clojure.core/update and int-map/update (in data.int-map and in Tom's namespace compiler-update-not-referenced-bug.core) show things look fine when printed inside data.int-map, and in Tom's namespace when not doing the uberjar, but when doing the uberjar, test, int-map/update is unbound in Tom's namespace.

In case it makes a difference, my testing was done with Mac OS X 10.9.5, Leiningen 2.5.0 on Java 1.7.0_45 Java HotSpot(TM) 64-Bit Server VM

Comment by Nicola Mometto [ 25/Nov/14 3:44 PM ]

Tom, I've opened a ticket with a patch fixing the AOT issue: http://dev.clojure.org/jira/browse/CLJ-1604

Comment by Kevin Downey [ 03/Jun/16 5:09 AM ]

the consequences of this bug can be very hard to track back to this bug. it would be really nice to get it fixed in someway.

(defmulti update identity)
... pages of other code ...
(defmethod update :foo [_])

will throw a compiler error on the defmethod saying update is unbound

Comment by Sean Corfield [ 18/Aug/16 4:02 PM ]

This looks very similar to the bug I ran into recently with Encore and an early Alpha of Clojure 1.9.0 where a new core predicate had been introduced, causing a warning from Encore which defined a function with the same name – but use of the code produced an error that the var was unbound. In that case it was a series of defn forms inside a do form – done to put a reader conditional around a group of function definitions. I lifted the conflicting functions out of the do and the error went away (but the warning remained, as expected). You can see the PR I submitted to Encore showing the code rearrangement: https://github.com/ptaoussanis/encore/pull/26/commits/040bf1be99eee79cbbcb7cc10ed37aa0a1e7ec17

Adding those functions to :refer-clojure :exclude instead solved the problem "properly" (and made the warning go away, obviously).





[CLJ-1142] Incorrect divide-by-zero error with floating point numbers Created: 08/Jan/13  Updated: 18/Aug/16

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

Type: Defect Priority: Minor
Reporter: Tim McCormack Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: math

Approval: Triaged

 Description   

The unary call for clojure.core// treats a dividend of 0.0 differently than the binary call, likely due to inlining.

(/ 0.0) ;; java.lang.ArithmeticException: Divide by zero
(/ 1 0.0) ;;= Infinity
(/ 1 (identity 0.0)) ;; java.lang.ArithmeticException: Divide by zero


 Comments   
Comment by Tim McCormack [ 08/Jan/13 11:22 PM ]

The relevant code seems to be this in clojure.lang.Numbers/divide:

if(yops.isZero((Number)y))
  throw new ArithmeticException("Divide by zero");

Making Numbers/divide be more restrictive than double arithmetic seems like a bug; explicitly throwing an ArithmeticException instead of letting the JVM figure it just seems like more work than necessary.

Comment by Casey Marshall [ 17/Aug/16 11:17 PM ]

I think the issue here is that in clojure.lang.Numbers, different variants of divide have different semantics. Numbers.divide(Object, Object) performs the isZero check, but Numbers.divide(long, double) does not (it just uses Java's division operator, which since the denominator is a double with value 0.0, produces Infinity).

A statement like (/ 1 0.0) gets compiled to call Numbers.divide(long, double), and thus produces Infinity. If the second argument is a function call or a var, it looks like an Object, so it gets compiled to use Numbers.divide(Object, Object), and that call throws when the second arg is zero (actually it compiles to a call to Numbers.divide(long, Object), but that just boxes the first argument and calls the other variant).

It does seem incorrect to have different semantics for division based on the inferred type at compile time; however, I don't know if this affects any other instance of division except divide-by-zero, so it's possibly not a practical problem.

Comment by Tim McCormack [ 18/Aug/16 6:48 AM ]

I don't know if this affects any other instance of division except divide-by-zero, so it's possibly not a practical problem.

I regard code that only fails sometimes as worse, because then bugs are more likely to get caught in production instead of development.





[CLJ-1793] Reducer instances hold onto the head of seqs Created: 05/Aug/15  Updated: 17/Aug/16

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

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

1.8.0-alpha2 - 1.8.0-alpha4


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

 Description   

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

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

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

Trickier example from CLJ-1250 that doesn't clear `this` in nested loop:

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

Problem: #'reducer closes over a collection in order to reify CollReduce, and the closed-over collection reference is never cleared. When code attempts to reduce over this anonymous transformed collection, it will realize the tail while the head is stored in the closed-over.

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

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

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

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

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

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

Patch: clj-1793-2.patch

Screened by: Alex Miller



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

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

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

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

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

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

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

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

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

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

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

See the commit message for an extensive-ish comment.

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

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

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

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

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

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

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

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

Comment by Nicola Mometto [ 23/Mar/16 7:34 AM ]

Just a note that the original ticket for this issue had 10 votes

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

The following code currently eventually causes an OOM to happen, the patch in this ticket correctly helps not holding onto the collection and doesn't cause memory to run infinitely

Before patch:

user=> (defn range* [x] (cons x (lazy-seq (range* (inc x)))))
#'user/range*
user=> (reduce + 0 (eduction (range* 0)))
OutOfMemoryError Java heap space  clojure.lang.RT.cons (RT.java:660)

After patch:

user=> (defn range* [x] (cons x (lazy-seq (range* (inc x)))))
#'user/range*
user=> (reduce + 0 (eduction (range* 0)))
;; runs infinitely without causing OOM




[CLJ-1985] with-gen of conformer loses unform fn Created: 21/Jul/16  Updated: 16/Aug/16  Resolved: 16/Aug/16

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: spec

Attachments: Text File conformer-with-gen.patch    
Patch: Code and Test
Approval: Ok

 Description   
(def ex (s/conformer inc dec))
(s/conform ex 1) ;; 2
(s/unform ex 2)  ;; 1
(def exc
  (s/with-gen
    (s/conformer inc dec)
    (fn [] (s/gen int?))))
(s/conform exc 1) ;; 2
(s/unform exc 2) ;; fails, no unformer

Cause: with-gen doesn't re-apply the unform fn to the new spec

Patch: conformer-with-gen.patch



 Comments   
Comment by Alex Miller [ 16/Aug/16 8:47 AM ]

Committed for next alpha.





[CLJ-2003] Nesting cat inside ? causes unform to return nested result Created: 11/Aug/16  Updated: 15/Aug/16

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

Type: Defect Priority: Major
Reporter: Sam Estep Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 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)


 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}]




[CLJ-2002] StackOverflowError in clojure.spec Created: 11/Aug/16  Updated: 15/Aug/16

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

Type: Defect Priority: Major
Reporter: Michiel Borkent Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 Description   

(s/conform (s/* (s/alt :n (s/* number?) :s (s/* string?))) [[1 2 3]])

causes:

StackOverflowError clojure.core/implements? (core_deftype.clj:539)



 Comments   
Comment by Phil Brown [ 14/Aug/16 9:50 PM ]

While the following isn't super useful, it causes one too:

user=> (s/conform (s/+ (s/? any?)) [:a])

StackOverflowError   clojure.lang.RT.first (RT.java:683)




[CLJ-1544] AOT bug involving namespaces loaded before AOT compilation started Created: 01/Oct/14  Updated: 15/Aug/16

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

Type: Defect Priority: Critical
Reporter: Allen Rohner Assignee: Unassigned
Resolution: Unresolved Votes: 13
Labels: aot

Attachments: Text File 0001-CLJ-1544-force-reloading-of-namespaces-during-AOT-co.patch     Text File 0001-CLJ-1544-force-reloading-of-namespaces-during-AOT-co-v2.patch     Text File 0001-CLJ-1544-force-reloading-of-namespaces-during-AOT-co-v3.patch     Text File 0001-CLJ-1641-disallow-circular-dependencies-even-if-the-.patch    
Patch: Code
Approval: Incomplete

 Description   

If namespace "a" that is being AOT compiled requires a namespace "b" that has been loaded but not AOT compiled, the classfile for that namespace will never be emitted on disk, causing errors when compiling uberjars or in other cases.

A minimal reproducible case is described in the following comment: http://dev.clojure.org/jira/browse/CLJ-1544?focusedCommentId=36734&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-36734

Other examples of the bug:
https://github.com/arohner/clj-aot-repro
https://github.com/methylene/class-not-found

A real issue triggered by this bug: https://github.com/cemerick/austin/issues/23

Related ticket: CLJ-1641 contains descriptions and comments about some potentially unwanted consequences of applying proposed patch 0001-CLJ-1544-force-reloading-of-namespaces-during-AOT-co-v3.patch

Approach: The approach taken by the attached patch is to force reloading of namespaces during AOT compilation if no matching classfile is found in the compile-path or in the classpath

Patch: 0001-CLJ-1544-force-reloading-of-namespaces-during-AOT-co-v3.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 04/Dec/14 12:45 PM ]

Possibly related: CLJ-1457

Comment by Nicola Mometto [ 05/Dec/14 4:51 AM ]

Has anyone been able to reproduce this bug from a bare clojure repl? I have been trying to take lein out of the equation for an hour but I don't seem to be able to reproduce it – this makes me think that it's possible that this is a lein/classlojure/nrepl issue rather than a compiler/classloader bug

Comment by Nicola Mometto [ 06/Dec/14 4:20 PM ]

I was actually able to reproduce and understand this bug thanks to a minimal example reduced from a testcase for CLJ-1413.

>cat error.sh
#!/bin/sh

rm -rf target && mkdir target

java -cp src:clojure.jar clojure.main - <<EOF
(require 'myrecord)
(set! *compile-path* "target")
(compile 'core)
EOF

java -cp target:clojure.jar clojure.main -e "(use 'core)"

> cat src/core.clj
(in-ns 'core)
(clojure.core/require 'myrecord)
(clojure.core/import myrecord.somerecord)

>cat src/myrecord.clj
(in-ns 'myrecord)
(clojure.core/defrecord somerecord [])

> ./error.sh
Exception in thread "main" java.lang.ExceptionInInitializerError
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.classForName(RT.java:2113)
	at clojure.lang.RT.classForName(RT.java:2122)
	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__5403.invoke(core.clj:5808)
	at clojure.core$load.doInvoke(core.clj:5807)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5352.invoke(core.clj:5653)
	at clojure.core$load_lib.doInvoke(core.clj:5652)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5691)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:630)
	at clojure.core$use.doInvoke(core.clj:5785)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at user$eval212.invoke(NO_SOURCE_FILE:1)
	at clojure.lang.Compiler.eval(Compiler.java:6767)
	at clojure.lang.Compiler.eval(Compiler.java:6730)
	at clojure.core$eval.invoke(core.clj:3076)
	at clojure.main$eval_opt.invoke(main.clj:288)
	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.io.FileNotFoundException: Could not locate myrecord__init.class or myrecord.clj on classpath.
	at clojure.lang.RT.load(RT.java:443)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5403.invoke(core.clj:5808)
	at clojure.core$load.doInvoke(core.clj:5807)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5352.invoke(core.clj:5653)
	at clojure.core$load_lib.doInvoke(core.clj:5652)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5691)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5774)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at core__init.load(Unknown Source)
	at core__init.<clinit>(Unknown Source)
	... 33 more

This bug also has also affected Austin: https://github.com/cemerick/austin/issues/23

Essentially this bug manifests itself when a namespace defining a protocol or a type/record has been JIT loaded and a namespace that needs the protocol/type/record class is being AOT compiled later. Since the namespace defining the class has already been loaded the class is never emitted on disk.

Comment by Nicola Mometto [ 06/Dec/14 6:51 PM ]

I've attached a tentative patch fixing the issue in the only way I found reasonable: forcing the reloading of namespaces during AOT compilation if the compiled classfile is not found in the compile-path or in the classpath

Comment by Nicola Mometto [ 06/Dec/14 7:30 PM ]

Updated patch forces reloading of the namespace even if a classfile exists in the compile-path but the source file is newer, mimicking the logic of clojure.lang.RT/load

Comment by Nicola Mometto [ 06/Dec/14 7:39 PM ]

Further testing demonstrated that this bug is not only scoped to deftypes/defprotocols but can manifest itself in the general case of a namespace "a" requiring a namespace "b" already loaded, and AOT compiling the namespace "a"

Comment by Tassilo Horn [ 08/Dec/14 4:46 AM ]

I'm also affected by this bug. Is there some workaround I can apply in the meantime, e.g., by dictating the order in which namespaces are going to be loaded/compiled in project.clj?

Comment by Nicola Mometto [ 15/Dec/14 10:58 AM ]

Tassilo, if you don't have control over whether or not a namespace that an AOT namespace depends on has already been loaded before compilation starts, requiring those namespaces with :reload-all should be enough to work around this issue

Comment by Tassilo Horn [ 15/Dec/14 11:36 AM ]

Nicola, thanks! But in the meantime I've switched to using clojure.java.api and omit AOT-compilation. That works just fine, too.

Comment by Michael Blume [ 15/Dec/14 5:05 PM ]

Tassilo, that's often a good solution, another is to use a shim clojure class

(ns myproject.main-shim (:gen-class))

(defn -main [& args]
  (require 'myproject.main)
  ((resolve 'myproject.main) args))

then your shim namespace is AOT-compiled but nothing else in your project is.

Comment by Tassilo Horn [ 16/Dec/14 1:07 AM ]

Thanks Michael, that's a very good suggestion. In fact, I've always used AOT only as a means to export some functions to Java-land. Basically, I did as you suggest but required the to-be-exported fn's namespace in the ns-form which then causes AOT-compilation of that namespace and its own deps recursively. So your approach seems to be as convenient from the Java side (no need to clojure.java.require `require` in order to require the namespace with the fn I wanna call ) while still omitting AOT. Awesome!

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

I'm marking this as incomplete to prevent further screening until the bug reported here: http://dev.clojure.org/jira/browse/CLJ-1620?focusedCommentId=37232&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-37232 is figured out

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

Fixed the patch, I'm re marking the tickets as Vetted as it was before.

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

This patch is being rolled back for 1.7.0-alpha6 pending further investigation into underlying problems and possible solutions.

Comment by Colin Fleming [ 19/Jan/15 4:41 AM ]

I'm not 100% sure, but this looks a lot like Cursive issue 369. It had a case that I could reproduce with JDK 7 but not JDK 8, has the same mysterious missing namespace class symptom, and involves mixed AOT/non-AOT namespaces. However it's happening at runtime, not at compile time, which doesn't seem consistent.

Comment by Alex Miller [ 19/Jan/15 7:29 AM ]

My error report above was incorrectly tied to this issue (see CLJ-1636). I will delete the comment.

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

Since ticket CLJ-1641 has been closed, I'll repost here a comment I posted in that ticket + the patch I proposed, arguing why I think the patch I proposed for this ticket should not have been reverted:

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

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

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

This way, both under JIT and AOT code like

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

will fail with a circular depdenency error.

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

Comment by Stuart Halloway [ 15/Aug/16 1:35 PM ]

This is actually a runtime problem, not a compilation problem.

AOTed Clojure code cannot see JITed Clojure classes (vars are fine), because of the rules of Java classloading. JITed code is loaded by a DynamicClassLoader, which delegates up to the classpath loader for AOTed code.

The person who runs a particular Clojure app can solve this problem by making sure their own consumption of AOT compilation is "infectious", i.e. if you want to AOT-compile library A which uses library B, then you need to AOT-compile library B as well.

I think that attempts to have the Clojure compiler magically implement the "infectious" rule above will cause more problems than they solve, and that we should close this ticket and provide good guidance for tools like lein and boot.

Comment by Michael Sperber [ 15/Aug/16 2:24 PM ]

This problem occurs within the compilation of a single library/project, so I don't think this can be solved by simple usages of Leiningen or Boot.





[CLJ-2001] Invalid conversion from BigDecimal to long using clojure.core/long Created: 09/Aug/16  Updated: 09/Aug/16

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

Type: Defect Priority: Major
Reporter: Eugene Aksenov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math
Environment:

Ubuntu Linux 15


Attachments: Text File clj-2001.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Trying to convert from BigDecimal to long

(long 201608081812113241M)
=> 201608081812113248                  ;; not really our number

let's just use BigDecimal.longValue()

(.longValue 201608081812113241M)
=> 201608081812113241                  ;; ok, correct value

looking into clojure.lang.RT and suspecting incorrect conversion chain

(.longValue (.doubleValue 201608081812113241M))
=> 201608081812113248                  ;; yep, incorrect

Cause: long cast from BigDecimal will use Number.longValue(), which in this case produces an incorrect value even though the conversion is possible. The javadoc indicates that this call is equivalent to a double to long conversion and is potentially lossy in several ways.

Approach: add explicit case in long cast to handle BigDecimal and instead call longValueExact(). Patch adds additional cast tests for some BigInteger and BigDecimal values. The unchecked-long cast does not seem to be affected (returned the proper value with no changes).

Questions: while it may be confusing, the incorrect result may actually be the one that is consistent with Java. unchecked-long would give the expected result and may be the better choice for the example here. So it's possible that we should NOT apply this patch and instead do nothing. If we do move forward with the patch, we may want to also apply an equivalent change to call byteValueExact(), shortValueExact(), intValueExact(), and toBigIntegerExact() in the appropriate places as well.

Patch: clj-2001.patch



 Comments   
Comment by Alex Miller [ 09/Aug/16 8:14 AM ]

Yeah, RT.longCast() doesn't seem to explicitly handle BigDecimal.

Comment by Ghadi Shayban [ 09/Aug/16 10:07 AM ]

Patch seems like it may negatively affect inlining

Comment by Alex Miller [ 09/Aug/16 7:36 PM ]

Indeed that's a possibility, although I think it's probably rare in this case.





[CLJ-1044] Enable refering to ->type inside deftype Created: 18/Aug/12  Updated: 09/Aug/16

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

Type: Enhancement Priority: Trivial
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: deftype

Attachments: File 001-enable-factory-ctor-inside-deftype.diff    
Patch: Code
Approval: Triaged

 Description   

Inside a defrecord body it's possible to refer to ->type-ctor but that is not possible inside deftype.

This patch adds an implicit declare, as done in defrecord making it possible to use the ->type-ctor inside deftype methods



 Comments   
Comment by Timothy Baldridge [ 03/Dec/12 11:29 AM ]

Seems valid. Vetting.

Comment by Sam Estep [ 09/Aug/16 10:34 AM ]

Will this be incorporated soon? It's awkward to have to explicitly declare the factory function when defining, e.g., data structures (example). Also, the current situation violates the principle of least astonishment; while working through this tutorial, I was quite surprised to find that defrecord does implicitly declare the factory functions, which contradicted my prior experience with deftype.

Comment by Alex Miller [ 09/Aug/16 7:34 PM ]

No enhancements are considered critical so it's hard for me to say when this will get evaluated. I've bumped it one step down the process at least.

Comment by Sam Estep [ 09/Aug/16 7:39 PM ]

Thank you, Alex! I completely understand that this isn't a particularly important issue; nonetheless, it is encouraging to see it move closer to being fixed.





[CLJ-2000] (transduce) does an unexpected extra step Created: 06/Aug/16  Updated: 07/Aug/16  Resolved: 07/Aug/16

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

Type: Defect Priority: Minor
Reporter: Vadim Liventsev Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

https://stackoverflow.com/questions/38809642/applying-a-transducer-directly-and-with-transduce-yield-different-results/38809928#38809928

(reduce) calls (reducer aggregate element) for every element in the collection. A total of n calls for a collection of n elements.

(transduce) calls (reducer aggregate element) for every element and then for some reason calls (reducer aggregate) again, making n+1 calls. As a result, (transduce) doesn't work as expected with .

Is it a bug?



 Comments   
Comment by Kevin Downey [ 06/Aug/16 10:21 PM ]

this is a feature of transduce. the docstring for transduce says "f should be a reducing step function that accepts both 1 and 2 arguments, if it accepts only 2 you can add the arity-1 with 'completing'." which, if you know about the completing arity of reducing functions clues you in, but if you don' the docstring is not very helpful about it.

Comment by Alex Miller [ 07/Aug/16 11:42 AM ]

As Kevin said in the comments, the question assumes things that are not accurate. Everything here is working as expected/designed.





[CLJ-1999] Infinity and -Infinity do not equal literal versions if within a data structure Created: 03/Aug/16  Updated: 03/Aug/16  Resolved: 03/Aug/16

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

Type: Defect Priority: Minor
Reporter: Darrick Wiebe Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

The first statement below is correctly true, but the second one is false.

(= [0 Double/POSITIVE_INFINITY] [0 Double/POSITIVE_INFINITY])

(= [0 Double/POSITIVE_INFINITY] (clojure.edn/read-string "[0 Infinity]"))



 Comments   
Comment by Alex Miller [ 03/Aug/16 1:37 PM ]

The problem here is that Infinity is read as a symbol, not as Double/POSITIVE_INFINITY.

I think these issues are captured better in http://dev.clojure.org/jira/browse/CLJ-1074 so I'm going to mark this as a dupe of that one.





[CLJ-1998] clj.spec: improve boolean kw option naming Created: 03/Aug/16  Updated: 03/Aug/16

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

Type: Enhancement Priority: Trivial
Reporter: Max Penet Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec


 Description   

We have a mix of boolean keyword options with and without trailing "?" at the moment. It would be good to settle to 1 style, hopefully the one with the trailing "?".

Ex: in map-of we have :conform-keys, in double-in: NaN? and :infinite? and possibly others.






[CLJ-1997] Macros cannot reliably detect usage of locals Created: 02/Aug/16  Updated: 02/Aug/16

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

Type: Enhancement Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: macro


 Description   

Problem

The motivating problem is the implementation of gen/let in test.check (see also TCHECK-98).

A common usage of gen/let might look something like this:

(gen/let [a gen-a
          b gen-b]
  (f a b))

The crucial characteristic of this code is that the generator for b does not depend on the value a (though in general it could). Because of this independence, the ideal expansion is:

(gen/fmap 
  (fn [[a b]] (f a b)) 
  (gen/tuple gen-a gen-b))

However, because gen/let cannot, in general, tell whether or not the expression for the generator for b depends on a, it needs to fallback to a more general expansion:

(gen/fmap
  (fn [[a b]] (f a b))
  (gen/bind 
    gen-a
    (fn [a]
      (gen/tuple (gen/return a) gen-b))))

Using gen/bind greatly reduces shrinking power, and so it's best to avoid it when possible.

A knowledgeable user could get around this by using gen/tuple explicitly, e.g.:

(gen/let [[a b] (gen/tuple gen-a gen-b)]
  (f a b))

But I think most users would prefer not to have to think about these things.

Possible Solutions

tools.analyzer

tools.analyzer is probably adequate, but is a large dependency for a library.

a subset of tools.analyzer

Nicola has mentioned the idea of carving out some subset of the analyzer that would be sufficient for this case, and that might be the best option.

a mechanism for macroexpanding a macro body

I believe if there were a robust mechanism for a macro to fully macroexpand an expression that this problem would be easier (clojure.core/macroexpand and friends have a few known incorrectnesses) – a simple tree-seq over the expanded expression could prove that a local is not used (though a naive approach might falsely conclude that a local *is* used, which might be an acceptable compromise for the test.check case, and otherwise a robust code walker should not be difficult to implement on expanded code).

I believe zach's riddley library does something like this, and depending on riddley would probably be the best option for a non-contrib library, but is not an acceptable dependency for a contrib library.






[CLJ-889] Specifically allow '.' inside keywords Created: 01/Dec/11  Updated: 01/Aug/16

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: keywords, reader


 Description   

The documentation for keywords (on page http://clojure.org/reader) specifically states that '.' is not allowed as part of a keyword name; however '.' is specifically useful. For example, several web frameworks for Clojure use keywords to represent HTML elements, using CSS selector syntax (i.e., :div.important is equivalent to <div class='important'>).

In any case, the use of '.' is not checked by the reader and it is generally useful.

I would like to see '.' officially allowed (in the documentation). Further, I'd like to see additional details about which punctuation characters are allowed (my own web framework uses '&', '?' and '>' inside keywords for various purposes ... again, current reader implementation does not forbid this, but if a future reader will reject it, I'd like to know now).



 Comments   
Comment by Howard Lewis Ship [ 08/Dec/11 3:37 PM ]

To clarify, Hiccup and Cascade both use keywords containing '#' and '.' Cascade goes further, using '&' (to represent HTML entities), '>', and (possibly in the future) '?'.

Comment by Devin Walters [ 20/Oct/12 6:46 PM ]

I think the EDN spec mitigates some of the concern, but as of yet the official clojure.org reader documentation does not reflect the language used in the description of EDN. Where does EDN stand right now? Can the description being used on the github page be pulled over to clojure.org?

References:

Comment by Howard Lewis Ship [ 15/Apr/13 5:56 AM ]

Unfortunately, the EDN specification does not mention '>'.

Comment by Vitalie Spinu [ 01/Aug/16 2:35 PM ]

It's worth noting that the most common prefix character '#' is allowed in keywords by the reader, but other prefix characters '`', '@' and '~' are not. None of these characters are allowed as part of symbols proper.





[CLJ-1996] clojure.spec stubs don't cooperate with clojure.spec.test/check Created: 31/Jul/16  Updated: 31/Jul/16

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

Type: Defect Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

This is just like CLJ-1949, but for stubs instead of higher-order-function arguments.

The solution is more difficult, though, since cst/check and cst/instrument can be called/used seperately.

My only idea is to have a dynamic var where the two can coordinate. Stubs would use gen/generate when not called during testing, but in the context of a call to cst/check the dynamic var would contain an alternate implementation that works similarly to the patch in CLJ-1949.

I'd be happy to prepare a patch with that implementation (or any other) if desired.






[CLJ-1994] the-ns / clojure.lang.Namespace/find should handle nil values Created: 29/Jul/16  Updated: 31/Jul/16  Resolved: 31/Jul/16

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

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


 Description   

After some refactoring, I had the (broken) code:

(test-all-vars (find-ns 'missing.ns))

when I ran my tests, I got this error:

...
Caused by: java.lang.NullPointerException
	at java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:936)
	at clojure.lang.Namespace.find(Namespace.java:188)
	at clojure.core$find_ns.invokeStatic(core.clj:4000)
	at clojure.core$the_ns.invokeStatic(core.clj:4030)
	at clojure.core$ns_interns.invokeStatic(core.clj:4077)
	at clojure.test$test_all_vars.invokeStatic(test.clj:736)
	at clojure.test$test_all_vars.invoke(test.clj:736)
...

I expected there would be an exception thrown that the namespace could not be found.

Looking into it further, it seems like the issue is that find-ns was called twice in this code, once at the top level on the non-existent namespace which returned nil, and then a second time in the-ns with nil as the value. The second nil blows up inside clojure.lang.Namespace/find. It seems like there should be a nil check somewhere along the way, probably either in the-ns, or clojure.lang.Namespace/find, which would then throw a descriptive error message.



 Comments   
Comment by Alex Miller [ 29/Jul/16 7:58 AM ]

find-ns says "Returns the namespace named by the symbol or nil if it doesn't exist." and you're getting nil in this case.

So can't this just be simplified to (test-all-vars nil)? Why should we expect that to work?

Comment by Daniel Compton [ 31/Jul/16 5:15 AM ]

You're right, I'm not expecting it to work, I'm suggesting that we throw a more descriptive error. Thinking about this over the last few days, there are a few options:

Add a nil check in the-ns so that an exception is also thrown on nil passed as a namespace:

;; Something with this intent:
(if (instance? clojure.lang.Namespace x)
    x
    (or (and x (find-ns x)) (throw (Exception. (str "No namespace: " x " found")))))

Add a nil check in find-ns:

(defn find-ns
  "Returns the namespace named by the symbol or nil if it doesn't exist."
  {:added  "1.0"
   :static true}
  [sym] (when (some? sym) (clojure.lang.Namespace/find sym)))

Add a nil check in clojure.lang.Namespace/find:

public static Namespace find(Symbol name) {
        // If nil throw more descriptive error than NPE
        return (Namespace)namespaces.get(name);
    }

Thinking some more about it, all of these changes could be breaking to people who relied on the old behaviour. It's not clear to me whether this is worth continuing. Thoughts?

Comment by Alex Miller [ 31/Jul/16 8:16 AM ]

I think find-ns, Namespace/find, and the-ns are behaving as documented and intended. If anything, having a spec for test-all-vars would catch this more explicitly.

(s/fdef clojure.test/test-all-vars :args #(instance? clojure.lang.Namespace %))
(st/instrument 'clojure.test/test-all-vars)

;; results in:
user=> (clojure.test/test-all-vars (find-ns 'foo))
ExceptionInfo Call to #'clojure.test/test-all-vars did not conform to spec:
val: (nil) fails at: [:args] predicate: (instance? clojure.lang.Namespace %)
:clojure.spec/args  (nil)
:clojure.spec/failure  :instrument
:clojure.spec.test/caller  {:file "NO_SOURCE_FILE", :line 11, :var-scope user/eval23}
  clojure.core/ex-info (core.clj:4724)

which would have pointed you pretty precisely to the problem. But I don't think it's worth keeping this ticket open re the spec as we are working on specs via other means.





[CLJ-1995] Improved docstring for explain-data Created: 30/Jul/16  Updated: 30/Jul/16

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

Type: Enhancement Priority: Minor
Reporter: Marshall Abrams Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, spec

Approval: Triaged

 Description   

In 1.9.0-alpha10, the docstring for explain-data doesn't mention or describe the meaning of some standard keys/values of its return value, and the use of "path" in the docstring could be clarified to avoid conflation with file paths or namespace paths. Here is the current docstring:

Given a spec and a value x which ought to conform, returns nil if x conforms, else a map with at least the key ::problems whose value is a collection of problem-maps, where problem-map has at least :path :pred and :val keys describing the predicate and the value that failed at that path.

Here is a possible replacement:

Given a spec and a value x which ought to conform, returns nil if x conforms, else a map with at least the key ::problems whose value is a collection of problem-maps, where problem-map has at least :path :pred and :val keys describing the predicate and the value that failed at that path (through possibly embedded specs). The map may also contain a :via key for specs that failed, an :in key for data key(s) of the value that failed, and a :reason key for a string describing the reason for failure.

This differs from the existing docstring in two ways:

1. It inserts "(through possibly embedded specs)" at the end of the existing dostring to clarify and disambiguate the meaning of "path" here.

2. It adds an additional sentence describing the :via, :in, and :reason keys.






[CLJ-1991] ClassCastException in Compiler.java Created: 26/Jul/16  Updated: 27/Jul/16  Resolved: 26/Jul/16

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

Type: Defect Priority: Major
Reporter: Jeremy Betts Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Linux Zulu JDK



 Description   

on line 272 of Compiler.java there is a class cast exception when the value from the system property is an Integer. We cannot upgrade from 1.6 until this is fixed

it is probably best to do a toString() rather than a hard cast.

for (Map.Entry e : System.getProperties().entrySet())
{
String name = (String) e.getKey();
String v = (String) e.getValue();



 Comments   
Comment by Alex Miller [ 26/Jul/16 12:53 PM ]

Hi Jeremy,

This has been filed and declined in the past (see CLJ-1717) on the grounds that System property values are defined to be Strings and to do otherwise is an error in whomever set that property, not on Clojure's assumption.

Alex

Comment by Jeremy Betts [ 26/Jul/16 1:33 PM ]

You realize that you are basically locking my company at version 1.6 right?

This should be fixed from a code quality perspective regardless.

A) why do an unchecked casts to string rather than using toString() and
B) its a one line change!

This type of thinking is only going to stop the adoption of Clojure from being used as anything more than a toy language.

In the 'real world' other code has bugs and it cannot always be fixed. You should be writing robust code that doesn't 'assume' that the world it is in is a pristine environment.

Comment by Sean Corfield [ 26/Jul/16 10:39 PM ]

Jeremy, Java itself defines System property values as Strings: https://docs.oracle.com/javase/7/docs/api/java/util/Properties.html

Can you show how you are "locked" at Clojure version 1.6 because of this?

It's not like you can store non-String values in there:

user=> (System/setProperty "foo" 123)

ClassCastException java.lang.Long cannot be cast to java.lang.String  user/eval1227 (form-init3357403087479620173.clj:1)

That's on Clojure 1.6.0.

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

Sean, some (bad) Java libs do this - Properties extends Hashtable and thus it's possible to use the super-class put method to put a non-String value into the System properties map after startup - Java calls these "compromised" properties. Creating properties like this breaks many API assumptions in the Properties class and is a known Bad thing to do.

Jeremy, I'm sorry that this is an issue for you, but the problem here is really with the code that is setting the System property, not Clojure.

Comment by Jeremy Betts [ 27/Jul/16 9:00 AM ]

I realize that Clojure has a garbage in garbage out approach - but since this code is in a static {} block, where exactly should I put 'safety code' to ensure that no non String values exist in the system properties? The JVM controls when static blocks get run!

Obviously non stings can get in there since this issue has been reported before - Alex you even new the previous issue number off the top of your head! Any third party code could be doing it.

It would be wise not go blundering into the well known areas of bad design in Java.

The solution is to call toString() instead of doing a unchecked hard cast to (String). This is simply a good coding standard anyway.

can you give any real reason not to make this simple change? You can't even claim performance since this is a static block which gets run exactly once!

We all know that java isn't the best language, that is why we seek other languages like Clojure. But if those other languages force the worst features of Java to the forefront to cause pain why bother?

I would need a reliable work around to this issue that ensures it can never happen. But since the issue can reoccur at the fancy of the JVM executing the static code elsewhere, this is a total show stopper.

Comment by Jeremy Betts [ 27/Jul/16 9:23 AM ]

Also, attempting to remove values from system properties to mitigate this issue has that high risk of throwing a concurrent modification exception.

Comment by Jeremy Betts [ 27/Jul/16 12:20 PM ]

here is a simple fix that represents better java programming practice and makes the code faster and safer.

for (Map.Entry e : System.getProperties().entrySet())
{
String name = (String) e.getKey();
if (name.startsWith("clojure.compiler."))

{ String v = (String) e.getValue(); compilerOptions = RT.assoc(compilerOptions, RT.keyword(null, name.substring(1 + name.lastIndexOf('.'))), RT.readString(v)); }

}

Comment by Alex Miller [ 27/Jul/16 1:25 PM ]

What is putting bad properties in your system? Shouldn't you be spending your effort on the root cause?

Comment by Jeremy Betts [ 27/Jul/16 1:37 PM ]

an ancient mail library. I'm pushing on both bugs.

Why don't you want to fix yours? I'm sure you've spent more time fighting with JIRA issues than what it would take to fix it.

Comment by Alex Miller [ 27/Jul/16 2:34 PM ]

Because it's not our bug.

Comment by Jeremy Betts [ 27/Jul/16 2:41 PM ]

you assign values you never use. You obtain these values from a global static hashmap that anyone can add any value to, even though they "should" only add strings. You do an unchecked assignment. You have a bug.





[CLJ-1990] Add an async macro that behaves the same as ClojureScript's Created: 24/Jul/16  Updated: 26/Jul/16

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

Type: Enhancement Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: clojure.test, portability

Attachments: Text File clj-1990v1.patch    
Patch: Code and Test
Approval: Triaged

 Description   

We want to run the same tests between Clojure and ClojureScript. However some of our CLJS tests are asynchronous and require the use of the async macro. Clojure doesn't have the async macro, which makes test portability difficult. If we were to add the async macro, there are at least two possible routes to go down: imitating the async behaviour of cljs.test, or copying the implementation. If we were to imitate the behaviour, we could use the following async macro. It creates a promise, runs the body, blocks on the promise, and done delivers the promise, allowing the test to continue.

(defmacro async
  [done & body]
  `(let [p# (promise)
         ~done #(deliver p# nil)]
     ~@body
     (deref p#)))

This has the advantage that it integrates with the current way tests are run in Clojure, and doesn't require any of the surrounding tooling to be aware of the async testing.

Imitating the implementation would be much more invasive. It would probably mean changing the behaviour of run-tests to return nil like ClojureScript does, and a host of other changes. This means it is likely a non-starter.

I lean heavily towards the first option.

To answer the question "Why do we need this at all when it is a no-op?", this is useful so we can write the same tests in both Clojure and ClojureScript. It would be much more tricky to write without it.



 Comments   
Comment by Daniel Compton [ 26/Jul/16 12:12 AM ]

v1, patch and tests





[CLJ-1992] Add explanation for clojure.test-clojure.metadata/public-vars-with-docstrings-have-added failure Created: 26/Jul/16  Updated: 26/Jul/16  Resolved: 26/Jul/16

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

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

Patch: Code and Test

 Description   

While I was adding the async macro patch, I ran the tests and clojure.test-clojure.metadata/public-vars-with-docstrings-have-added failed. As someone unfamiliar to the codebase, it wasn't very obvious why it had failed, or what I had to do to fix it. I traced it back to understand I needed to add {:added "1.9"}. This is obvious in hindsight, but wasn't apparent from the test failure message. The attached patch adds an explanation of why the test failed.

Compare the test output from before and after:

[java] FAIL in (public-vars-with-docstrings-have-added) (metadata.clj:46)
     [java] expected: (= [] (remove (comp :added meta) public-vars-with-docstrings-not-generated))
     [java]   actual: (not (= [] (#'clojure.test/async)))
[java] FAIL in (public-vars-with-docstrings-have-added) (metadata.clj:46)
     [java] All public vars with docstrings must have :added metadata, e.g. {:added "1.0"}
     [java] expected: (= [] (remove (comp :added meta) public-vars-with-docstrings-not-generated))
     [java]   actual: (not (= [] (#'clojure.test/async)))


 Comments   
Comment by Alex Miller [ 26/Jul/16 5:33 PM ]

"public vars with docstrings have added" is the documentation, not going to bother with this

Comment by Daniel Compton [ 26/Jul/16 5:38 PM ]

It's not the documentation, it's the name of the test, and it wasn't very clear. Changing the test name to public-vars-with-docstrings-have-added-metadata would improve things. This is a small thing, but it tripped me up, and it seems like it could make contributing to Clojure a tiny bit nicer and easier.





[CLJ-1945] provide a way to write a map spec that disallows extra keys Created: 04/Jun/16  Updated: 24/Jul/16  Resolved: 04/Jun/16

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

Type: Enhancement Priority: Minor
Reporter: Chris Price Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

After reading the initial docs and tutorials, I was expecting that calling `conform` or `valid` with a `key` spec and a map that contained extra keys would indicate a spec failure, but it doesn't:

```clj
(spec/conform
(spec/keys :req [::foo ::bar])
{::foo "foo" ::bar "bar" ::baz "baz"})
=>
{:user.swagger-ui-service/foo "foo",
:user.swagger-ui-service/bar "bar",
:user.swagger-ui-service/baz "baz"}
```

Obviously this behavior is desirable in many situations, but perhaps there could also be another spec type, called `exact-keys` or something, that would fail in the above example because of the presence of the non-specified `::baz` key.

This seems like it would be particularly useful when specifying the return value for a function that is returning data for an HTTP endpoint, to make sure that the program isn't violating the API specification by including extraneous data in the response.

This can be achieved with the current `spec` by `and`ing together a `keys` spec with a custom predicate that does some set logic on the keys, but that is a little unwieldy and repetitive, and doesn't produce as nice of an error message as what could probably be done if it were built in.



 Comments   
Comment by Alex Miller [ 04/Jun/16 9:05 PM ]

I am declining this ticket as this was considered and intentionally not provided. Rich believes that maps should be open containers and that extra attributes should be allowed (similar philosophy behind having open records).

As you mention, there are other ways to add this constraint if desired.

Comment by Jan-Paul Bultmann [ 24/Jul/16 4:18 PM ]

I hope you guys reconsider at some point, this is the only reason why I'll stick with schema.





[CLJ-1987] Update clojure.java.javadoc to open JDK8 docs by default Created: 24/Jul/16  Updated: 24/Jul/16  Resolved: 24/Jul/16

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

Type: Enhancement Priority: Trivial
Reporter: Richard Hull Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: documentation

Attachments: Text File jdk8_javadoc.patch    

 Description   

The clojure.java.javadoc function opens up JavaSE 7 docs in a browser - looking at the source (https://github.com/clojure/clojure/blob/master/src/clj/clojure/java/javadoc.clj#L21-L24), this hasn't been updated for a few years.

The attached trivial patch to set the default to use the Java 8 docs



 Comments   
Comment by Alex Miller [ 24/Jul/16 6:32 AM ]

There is already a ticket/patch for this at http://dev.clojure.org/jira/browse/CLJ-1398 - any comments should go there.

Comment by Richard Hull [ 24/Jul/16 10:12 AM ]

Ah ok, I did a search in JIRA and didn't spot CLJ-1398 in the results + couldn't find anything committed in git, so assumed that this was something that was worth submitting. Anyway, thanks for the quick response,
regards
Richard





[CLJ-1986] Suppress printing namespace map literal syntax when only one namespaced key Created: 21/Jul/16  Updated: 21/Jul/16

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: maps, print

Attachments: Text File clj-1986.patch    
Patch: Code and Test
Approval: Prescreened

 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-1984] clojure.spec/double-in should allow strict greater-than, less-than tests Created: 20/Jul/16  Updated: 21/Jul/16

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

Type: Enhancement Priority: Minor
Reporter: Marshall Abrams Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec


 Description   

clojure.spec/double-in defines a spec that tests whether a double is greater than or equal to a minimum value and less than or equal to a maximum value. This seems like an arbitrary choice from the point of view of mathematics and practical concerns. Sometimes you need to test whether a double is greater than a minimum or less than a maximum. Example: The application will divide by the tested double later.

Of course we can add tests to double-in, e.g. like

(s/and (s/double-in :min 0.0 :max 1.0) #(not= 0.0 %))}}

but

#(and (> % 0.0) (<= % 1))

might be clearer if double-in's NaN and Infinity tests aren't needed.

Why not have a common interface to all four interval tests? Rather than four different spec functions, which is one option, I suppose, I suggest adding two keywords to double-in. When true, these would change the >= or <= tests to > or < tests:

:min-greater

(or? :min+, :min-greater-than, :greater-than-min, :strict-min, :min-open, or possibly :infinmum, :inf, but that could be misleading)

:max-less

(or :max- :max-less-than, :less-than-max, :strict-max, :max-open, or possibly :supremum, :sup etc.)

For example,

(s/valid? (s/double-in :min 0.0 :max 0.1 :min-greater true) 0.0)

would return false, but

(s/valid? (s/double-in :min 0.0 :max 0.1 :min-greater false) 0.0)

would return true.

Default values for these keywords should probably be false, for compatibility with the current definition of double-in.






[CLJ-1959] adding functions `map-vals` and `map-keys` Created: 14/Jun/16  Updated: 21/Jul/16

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

Type: Enhancement Priority: Major
Reporter: Hiroyuki Fudaba Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File map-mapper.patch     Text File map-mapper-v2.patch     Text File map-mapper-v3.patch    
Patch: Code
Approval: Triaged

 Description   

Many people have been writing a function to map values in HashMap:

Proposal: Add `map-keys` and `map-values` which: maps keys in HashMap, and map values in HashMap. They return HashMap as a result.

Workaround: Using function `reduce-kv` or ordinary `map` and `into` is a common solution, but they are confusing and types change, which makes it tricky and tedious.

Discussions: https://groups.google.com/forum/#!topic/clojure-dev/kkPYIl5qj0o



 Comments   
Comment by Hiroyuki Fudaba [ 14/Jun/16 11:22 AM ]

code and test for map-keys and map-vals

Comment by Nicola Mometto [ 14/Jun/16 1:05 PM ]

I propose those functions being called `update-vals` and `update-keys` rather than `map-vals` and `map-keys`

Comment by Alex Miller [ 14/Jun/16 2:03 PM ]

It's not worth bike-shedding names on this - Rich will have his own opinion regardless.

On the patch:

  • remove the :static metadata, that's not used anymore
  • needs docstrings, which should be written in the style of other Clojure docstrings. map is probably a good place to draw from.
  • rather than declare into, defer the definition of these till whatever it needs has been defined. There is no reason to add more declares for this.

There are other potential implementations - these should be implemented and compared for performance across a range of input sizes. In addition to the current approach, I would investigate:

  • reduce-kv with construction into a transient map. This allows the map to reduce itself (no seq caching needed) and avoid creating entries only to tear them apart again.
  • transducers with (into {} (map ...) m)

Also should consider

  • whether to build a k/v vector and convert to a map, or build a map directly (the former may be faster, not sure)
  • if building the map, how to construct the map entries (vector vs creating a mapentry object directly)
  • in map-keys, is there any open question when map generates new overlapping keys?
  • are there places in existing core code where map-keys/map-vals could be used (I am pretty certain there are)
Comment by Hiroyuki Fudaba [ 15/Jun/16 11:01 AM ]

Thanks for comments

> I propose those functions being called `update-vals` and `update-keys` rather than `map-vals` and `map-keys`
Maybe. But I name it `map-*` just for now, we can choose it later

about potential implementations:
I have tried several implementations, and seems to be the current implementation is the fastest.
You can see it here: https://github.com/delihiros/performance

about considerings:
> whether to build a k/v vector and convert to a map, or build a map directly (the former may be faster, not sure)
> are there places in existing core code where map-keys/map-vals could be used (I am pretty certain there are)
> if building the map, how to construct the map entries (vector vs creating a mapentry object directly)
I'll check which them as soon as possible. I haven't done it yet.

> in map-keys, is there any open question when map generates new overlapping keys?
I believe it should be overwritten by latter applied key and value.

Comment by Nathan Marz [ 15/Jun/16 11:35 AM ]

I've done quite a bit of investigation into this through building Specter. Here are some benchmarks of numerous ways of doing map-vals, including using Specter.

Code: https://github.com/nathanmarz/specter/blob/4778500e0370fb211f47ebf4d69ca64366117b6c/scripts/benchmarks.clj#L87
Results: https://gist.github.com/nathanmarz/bf571c9ed86bfad09816e17b9b6e59e3

A few comments:

  • Implementations that build and tear apart MapEntry's perform much worse.
  • Transients should be used for large maps but not for small ones.
  • This benchmark shows that the property of maintaining the type of the map in the output can be achieved without sacrificing performance (the test cases using Specter or "empty" have this property).
Comment by Hiroyuki Fudaba [ 11/Jul/16 3:27 AM ]

I've modified the implementation. It should be faster than before.

Comment by Steve Miner [ 20/Jul/16 10:46 AM ]

Implementations that call reduce-kv are not lazy so the documentation should be clarified in the proposed patch (map-mapper-v3.patch). Also, it's probably better to say "map" (as the noun) rather than to specify a particular concrete type "hash map".

Comment by Nicola Mometto [ 21/Jul/16 4:30 AM ]

map->map operations can't be lazy either way. Even if one implementation used lazy operations to iterate over the original map, the `into {}` would realize it later.





[CLJ-1982] Better explain reporting on a failed zero or one match with an embedded spec. Created: 18/Jul/16  Updated: 19/Jul/16

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

Type: Enhancement Priority: Minor
Reporter: Nick Jones Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

OSX, Java 8, Clojure 1.9.0-alpha10



 Description   

Problem:

When attempting to validate a vector containing an optional map, the spec will validate correctly if the vector contains a valid map. If however the optional map does not satisfy the spec misleading error messages are produced. It would be nice if on a partial match of an optional map that some indication of this would be given to the user.

Example REPL session to illustrate problem:

The optional nested map (:optional-nested-map) below fails validation because :nested-element-b is a string instead of an int however the explain report says the spec fails at the parent predicate: :user/vector-schema at: [:element-value] predicate: string?.

It would be more helpful for the user in this case if spec reported that the optional nested map at :optional-nested-map had failed due to ::nested-element-b failing the int? predicate.

user=> (require '[clojure.spec :as s])
nil
user=> (s/def ::nested-element-a string?)
:user/nested-element-a
user=> (s/def ::nested-element-b int?)
:user/nested-element-b
user=> (s/def ::nested-element-schema
          (s/keys :opt-un [::nested-element-a ::nested-element-b]))
:user/nested-element-schema
user=> (s/def ::vector-schema
         (s/cat :tag-kw               #{:tag}
                :optional-nested-map  (s/? (s/spec ::nested-element-schema))
                :element-value        string?))
:user/vector-schema
user=> (s/valid? ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b 10} "Element"])
true
user=> (s/valid? ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b ""} "Element"])
false
user=> (s/explain ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b ""} "Element"])
In: [1] val: {:nested-element-a "bla", :nested-element-b ""} fails spec: :user/vector-schema at: [:element-value] predicate: string?
nil
user=>


 Comments   
Comment by Alex Miller [ 18/Jul/16 7:43 AM ]

Can you update this description with a self-contained example that demonstrates the problem? It's too hard to repro and understand this larger example.

Comment by Nick Jones [ 19/Jul/16 3:30 AM ]

Hi,

Sorry I don't seem to have access to edit the description of the ticket after creation. Here is a simplified sample that I hope will help illustrate the case better.

When the optional nested map below fails validation because :nested-element-b is a string instead of an int the explain report says the spec fails at the parent predicate: :user/vector-schema at: [:element-value] predicate: string?.

As it is an optional map I could see how this would be the case. When no match is found it moves onto the next predicate in the parent.

That said I think it could be helpful (especially in a large optional nested data structure) that if a partial match is achieved that that could be reported to the user as a potential spot for the spec failing.

user=> (require '[clojure.spec :as s])
nil
user=> (s/def ::nested-element-a string?)
:user/nested-element-a
user=> (s/def ::nested-element-b int?)
:user/nested-element-b
user=> (s/def ::nested-element-schema
          (s/keys :opt-un [::nested-element-a ::nested-element-b]))
:user/nested-element-schema
user=> (s/def ::vector-schema
         (s/cat :tag-kw               #{:tag}
                :optional-nested-map  (s/? (s/spec ::nested-element-schema))
                :element-value        string?))
:user/vector-schema
user=> (s/valid? ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b 10} "Element"])
true
user=> (s/valid? ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b ""} "Element"])
false
user=> (s/explain ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b ""} "Element"])
In: [1] val: {:nested-element-a "bla", :nested-element-b ""} fails spec: :user/vector-schema at: [:element-value] predicate: string?
nil
user=>
Comment by Nick Jones [ 19/Jul/16 3:45 AM ]

Added simplified version of project archive matching comment at 2016-07-19.

Comment by Alex Miller [ 19/Jul/16 8:27 AM ]

Nick, I've given you edit rights here. Generally, we don't like to have external projects for repro - if you can boil it down to a few line example in the description, that would be ideal.

Comment by Nick Jones [ 19/Jul/16 8:15 PM ]

Thanks Alex. I've updated the description and removed the project attachments. I've also added a REPL session to the description to reproduce the problem in a standalone Clojure 1.9.0-alpha10 REPL.





[CLJ-1983] clojure.data/diff throws an exception when comparing map keys of different types when used on sorted maps Created: 19/Jul/16  Updated: 19/Jul/16  Resolved: 19/Jul/16

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

Type: Defect Priority: Major
Reporter: Thomas Scheiblauer Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

e.g.

(clojure.data/diff (sorted-map :foo 42) (sorted-map 1 42))
(clojure.data/diff (sorted-map :foo 42) (sorted-map "x" 42))
(clojure.data/diff (hash-map :foo 42) (sorted-map 1 42))
(clojure.data/diff (hash-map :foo 42) (sorted-map "x" 42))

will throw
java.lang.ClassCastException: java.lang.Long cannot be cast to clojure.lang.Keyword
while e.g.

(clojure.data/diff (hash-map :foo 42) (hash-map 1 42))
(clojure.data/diff (hash-map :foo 42) (hash-map "x" 2))
(clojure.data/diff (sorted-map :foo 42) (sorted-map :bar 42))

will not.

The same applies to ClojureScript with a different exception (e.g. "Error: Cannot compare :foo to 1")



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

This is the same root problem as CLJ-1242, so duping to that one.

Comment by Thomas Scheiblauer [ 19/Jul/16 10:30 AM ]

It's not exactly a duplicate since diff should work in any case regardless of (compare x y) not working in this situation (possibly by design?).
(= (sorted-map :foo 42) (sorted-map 1 42)) works by the way.
(compare (sorted-map :foo 42) (sorted-map 1 42)) throws the exception.
In my opinion this could (and maybe should) be fixed in diff.

Comment by Alex Miller [ 19/Jul/16 12:41 PM ]

The stack traces for the two tickets are identical. diff is not using compare, it's using =. (= (sorted-map :foo 42) (sorted-map 1 42)) throws.

user=> (clojure.data/diff (hash-map :foo 42) (sorted-map "x" 42))
ClassCastException java.lang.String cannot be cast to clojure.lang.Keyword  clojure.lang.Keyword.compareTo (Keyword.java:114)
user=> (pst *e)
ClassCastException java.lang.String cannot be cast to clojure.lang.Keyword
	clojure.lang.Keyword.compareTo (Keyword.java:114)
	clojure.lang.Util.compare (Util.java:153)
	clojure.lang.RT$DefaultComparator.compare (RT.java:280)
	clojure.lang.PersistentTreeMap.doCompare (PersistentTreeMap.java:311)
	clojure.lang.PersistentTreeMap.entryAt (PersistentTreeMap.java:298)
	clojure.lang.PersistentTreeMap.containsKey (PersistentTreeMap.java:94)
	clojure.lang.APersistentMap.equiv (APersistentMap.java:87)
	clojure.lang.Util.pcequiv (Util.java:124)
	clojure.lang.Util.equiv (Util.java:32)
	clojure.data/diff (data.clj:134)
	clojure.data/diff (data.clj:120)
	user/eval20 (NO_SOURCE_FILE:11)
Comment by Thomas Scheiblauer [ 19/Jul/16 1:28 PM ]

You are of course right as I can see clearly now.
I did overlook the asymmetrical behavior of '=' in context of a sorted map.
Please excuse my ignorance.





[CLJ-1981] `spec/merge` does not flow conformed values across preds per docstring Created: 13/Jul/16  Updated: 18/Jul/16  Resolved: 18/Jul/16

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

Type: Defect Priority: Major
Reporter: Moritz Ulrich Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: spec
Environment:

[org.clojure/clojure "1.9.0-alpha10"]


Approval: Vetted

 Description   

The order of specs passed to spec/merge affect the spec/conform behavior of the keys specified. This seem to happen only with non-prefixed keys via (spec/keys :req-un [..])

The following code snippet shows the broken/non-intuitive behavior:

(require '[clojure.spec :as s])

(s/def ::id (s/conformer str))
(s/def ::m (s/keys :req-un [::id]))

;; Correct behavior on ::id
(s/conform ::id 42)
;;=> "42"

;; Fine if unmerged
(s/conform ::m {:id 42})
;;=> {:id "42"}

;; Fine if merged with ::m in the *last* position
(s/conform (s/merge map? ::m) {:id 42})
;;=> {:id "42"}

;; Broken because `map?` is the last arg
(s/conform (s/merge ::m map?) {:id 42})
;;=> {:id 42}

;; Broken because another `s/keys` is used as the last argument
(s/conform (s/merge ::m (s/keys :req-un [::foo]))
           {:id 42 :foo 23})
;;=> {:id 42, :foo 23}


 Comments   
Comment by Alex Miller [ 13/Jul/16 8:36 AM ]

Perhaps a simpler pair of examples - the first should return the result of the second if conformed values are flowing through the predicates.

(s/conform
  (s/merge (s/map-of keyword? (s/or :s string? :n number?))
           map?)
  {:x "s"})
=> {:x "s"}
(s/conform
  (s/merge map?
           (s/map-of keyword? (s/or :s string? :n number?)))
  {:x "s"})
=> {:x [:s "s"]}
Comment by Alex Miller [ 18/Jul/16 9:04 AM ]

This is working as designed. s/merge should not flow conformed values. The docstring has been corrected in https://github.com/clojure/clojure/commit/d920ada9fab7e9b8342d28d8295a600a814c1d8a





[CLJ-1980] Unable to construct gen in indirectly recursive specs with s/every and derivations Created: 12/Jul/16  Updated: 12/Jul/16

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

Type: Defect Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

alpha-10



 Description   

Problem statement: Some spec implementations return no generator but nil, in their gen* implementation when their recursion-limit has been reached (e. g. s/or). Specs that implement composition of other specs sometimes respect getting no generator from other specs gen* and adjust behavior of their own gen* accordingly, sometimes to the extent of returning nothing themselves (e. g. s/or's gen* returns nil if of all of its branches specs also don't have a gen and otherwise uses only those gens it got). However, there are various specs that don't respect getting no generator from gen* (like s/every, s/map-of) and they are essential building blocks in many real world recursive specifications. They then end up throwing an exception "Unable to construct gen ...".

Here is a minimal example (not real world usecase illustration) of the problem with actual specs:

;; A ::B is an s/or with branches going through ::B recursively
(s/def ::B (s/or :A ::A))

;; An ::A is a map of keywords to ::Bs (or it is empty as recursive termination)

(s/def ::A (s/map-of keyword? ::B
                     :gen-max 3))

Valid values for the spec above (I can mail you a real usecase that enforces above pattern in which we parse an internal query DSL) are: {}, {:a {}}, {:foo {:bar {}}} etc.

The problem why the current implementation of spec fails to generate values for above spec is that ::A's map-of doesn't generate an empty map when ::B's gen* returns nil, but instead throws an exception. s/every and all derived specs are affected by this and there might be others.

Proposed fix: A spec's gen* impl must always respect other spec's gen* returning nil not by throwing but by either adjusting the returned gen or by returning nil itself so that the not-returning-gen behavior propagates back to the caller where an exception should be thrown instead.






[CLJ-1978] recursion-limit (still) not respected Created: 08/Jul/16  Updated: 12/Jul/16

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

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


 Description   

See the closed issue http://dev.clojure.org/jira/browse/CLJ-1964

Despite the hint in the comments there, this issue does not seem resolved in Alpha-9. Issuing

(s/def ::map-tree (s/map-of keyword? (s/or :tree ::map-tree :leaf nil?)))

hangs on my machine.

See also the other example I included on https://groups.google.com/forum/#!topic/clojure/IvKJc8dEhts, which immediately results in a StackOverflowError on my machine.



 Comments   
Comment by Leon Grapenthin [ 12/Jul/16 1:03 PM ]

As the author of CLJ-1964 I can't confirm this.

(binding [s/*recursion-limit* 1]
  (s/exercise ::map-tree))

... immediately generates.

Using the new :gen-max argument spec can also generate with a higher recursion limit in reasonable time

(s/def ::map-tree (s/map-of keyword? (s/or :tree ::map-tree :leaf nil?)
                            :gen-max 3))
(time (s/exercise ::map-tree))
"Elapsed time: 0.135683 msecs"

Note that :gen-max defaults to 20, so with 4 recursion steps this quickly ends up generating 20^5 3.2 million values





[CLJ-1966] :clojure.spec/invalid is not a valid :clojure.spec/any value Created: 21/Jun/16  Updated: 12/Jul/16

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

Type: Defect Priority: Major
Reporter: Alexander Kiel Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

(clojure.spec/valid? :clojure.spec/any :clojure.spec/invalid) returns false

This issue gets serious, if one likes to write specs for core functions like = which are used by spec itself. I observed this bug as I wrote a spec for assoc.

A possible solution could be to use an (Object.) sentinel internally and :clojure.spec/invalid only at the API boundary. But I have not thought deeply about this.



 Comments   
Comment by Alexander Kiel [ 24/Jun/16 9:48 AM ]

I have another example were the described issue arises. It's not possible to test the return value of a predicate suitable for conformer, because it should return :clojure.spec/invalid itself.

(ns coerce
  (:require [clojure.spec :as s]))

(s/fdef parse-long
  :args (s/cat :s (s/nilable string?))
  :ret (s/or :val int? :err #{::s/invalid}))

(defn parse-long [s]
  (try
    (Long/parseLong s)
    (catch Exception _
      ::s/invalid)))
Comment by Alexander Kiel [ 12/Jul/16 10:01 AM ]

No change in alpha 10 with the removal of :clojure.spec/any and introduction of any?.





[CLJ-1172] Cross-linking between clojure.lang.Compiler and clojure.lang.RT Created: 28/Feb/13  Updated: 12/Jul/16  Resolved: 12/Jul/16

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

Type: Defect Priority: Minor
Reporter: Yegor Bugayenko Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: None
Environment:

version 1.5.0-RC17



 Description   

This is my code (an example):

import clojure.lang.Compiler;
import clojure.lang.RT;
import clojure.lang.Var;

Compiler.load("(+ 5 %)");
Var foo = RT.var("bar", "foo");
Object result = foo.invoke(10);
assert result.toString().equals("15");

This is what I'm getting:

ava.lang.ExceptionInInitializerError
	at clojure.lang.Compiler.<clinit>(Compiler.java:47)
	at foo.main(Main.java:75)
Caused by: java.lang.NullPointerException
	at clojure.lang.RT.baseLoader(RT.java:2043)
	at clojure.lang.RT.load(RT.java:417)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.lang.RT.doInit(RT.java:447)
	at clojure.lang.RT.<clinit>(RT.java:329)
	... 36 more

The same code worked just fine with version 1.4. Looks like Compiler is using RT and RT is using Compiler, both statically.



 Comments   
Comment by Yegor Bugayenko [ 04/Mar/13 11:40 AM ]

I cross-posted this question to SO: http://stackoverflow.com/questions/15207596

Comment by Yegor Bugayenko [ 05/Mar/13 12:04 AM ]

calling RT.init() before Compiler.load() solves the problem

Comment by Andy Fingerhut [ 05/Mar/13 4:17 AM ]

Yegor, do you consider it OK to close this ticket as not being a problem, or at least one with a reasonable workaround?

Comment by Yegor Bugayenko [ 05/Mar/13 1:11 PM ]

Yes, of course. Let's close it.

Comment by Andy Fingerhut [ 05/Mar/13 6:14 PM ]

Ticket submitter agrees that this is not an issue, or that there is a reasonable workaround.

Comment by Andy Fingerhut [ 13/Mar/13 12:58 AM ]

This issue came up again on the Clojure group. https://groups.google.com/forum/?hl=en_US&fromgroups=#!topic/clojure/2xdLNMb9yyQ

I did some testing, and the issue did not exist in Clojure 1.5.0-RC3 and before, and it has existed since 1.5.0-RC4. There was only one commit between those two points:

https://github.com/clojure/clojure/commit/9b80a552fdabeabdd93951a625b55ae49c2f8d83

Maybe this new behavior is an intended consequence of that change. I don't know. In any case, it seems like perhaps the "No need to call RT.init() anymore" message might be outdated?

Comment by Andy Fingerhut [ 13/Mar/13 12:59 AM ]

Reopening since it came up again, and there is some more info known about the issue. I'll let someone who knows more about the issue decide whether to close it.

Comment by Edward [ 23/Mar/13 10:31 AM ]

Doing this RT.load("clojure/core"); at the top works avoids the message from RT.init()

Comment by Jean Niklas L'orange [ 21/Jun/13 10:36 AM ]

It seems like RT.load("clojure/core") does not hide the message anymore - at least not from 1.5.1.

Comment by Alex Maletz [ 11/Dec/15 7:52 AM ]

Tested with 1.7.0 and 1.8.0-RC3.
If I put RT.init() before, exception doesn't happen, but message "No need to call RT.init() anymore" is shown. If I remove RT.init(), exception happens. Yes, there is workaround like if I put RT.baseLoader() everything works
But this behavior looks a bit ugly - especially for those who starts with Clojure - therefore voting for this bug.

Comment by Alex Miller [ 12/Jul/16 8:53 AM ]

The only supported path to doing this is to use the public Java API for Clojure.

http://clojure.github.io/clojure/javadoc/clojure/java/api/Clojure.html





[CLJ-42] GC Issue 38: When using AOT compilation, "load"ed files are not reloaded on (require :reload 'name.space) Created: 17/Jun/09  Updated: 12/Jul/16  Resolved: 12/Jul/16

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

Type: Defect Priority: Major
Reporter: Anonymous Assignee: Stephen C. Gilardi
Resolution: Not Reproducible Votes: 0
Labels: None


 Description   
Reported by m...@kotka.de, Jan 07, 2009
What (small set of) steps will reproduce the problem?

1. Create a file src/foo.clj

cat >src/foo.clj <<EOF
(ns foo (:load "bar"))
EOF

2. Create a file src/bar.clj

cat >src/bar.clj <<EOF
(clojure.core/in-ns 'foo)
(def x 8)
EOF

3. Start Clojure Repl: java -cp src:classes clojure.main -r

4. Compile the namespace.

user=> (compile 'foo)
foo

5. Require the namespace
user=> (require :reload-all :verbose 'foo)
(clojure.core/load "/foo")
(clojure.core/load "/bar")

What is the expected output? What do you see instead?

6. Re-Require the namespace

user=> (require :reload-all :verbose 'foo)
(clojure.core/load "/foo")

Only the "master" file is loaded, but not the bar file.
Expected would have been to also load the bar file.
Changes to bar.clj are not reflected, and depending
on the setting (eg. using multimethods in foo from
a different namespace) code may be corrupted.

What version are you using?

SVN rev. 1195


 Comments   
Comment by Assembla Importer [ 24/Aug/10 6:44 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/42

Comment by Assembla Importer [ 24/Aug/10 6:44 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Assembla Importer [ 24/Aug/10 6:44 AM ]

richhickey said: Updating tickets (#42, #71)

Comment by Assembla Importer [ 24/Aug/10 6:44 AM ]

richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)

Comment by Alex Miller [ 12/Jul/16 8:26 AM ]

Retested with 1.8. In step 6, I see both foo and bar reloaded.





[CLJ-1152] PermGen leak in multimethods and protocol fns when evaled Created: 30/Jan/13  Updated: 12/Jul/16

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

Type: Defect Priority: Major
Reporter: Chouser Assignee: Unassigned
Resolution: Unresolved Votes: 9
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 ]

it might be better to split this in to two issues, because at a very abstract level the two issues are the "same", but concretely they are distinct (protocols don't really share code paths with multimethods), keeping them together in one issue seems like a recipe for a large hard to read patch

Comment by Kevin Downey [ 26/Jul/14 5:49 PM ]

naive-lru-method-cache-for-multimethods.diff replaces the methodCache in multimethods with a very naive lru cache built on PersistentHashMap and PersistentQueue

Comment by Kevin Downey [ 28/Jul/14 7:09 PM ]

naive-lru-for-multimethods-and-protocols.diff creates a new class clojure.lang.LRUCache that provides an lru cache built using PHashMap and PQueue behind an IPMap interface.

changes MultiFn to use an LRUCache for its method cache.

changes expand-method-impl-cache to use an LRUCache for MethodImplCache's map case

Comment by Kevin Downey [ 30/Jul/14 3:10 PM ]

I suspect my patch naive-lru-for-multimethods-and-protocols.diff is just wrong, unless MethodImplCache really is being used as a cache we can't just toss out entries when it gets full.

looking at the deftype code again, it does look like MethidImplCache is being used as a cache, so maybe the patch is fine

if I am sure of anything it is that I am unsure so hopefully someone who is sure can chime in

Comment by Nicola Mometto [ 31/Jul/14 11:02 AM ]

I haven't looked at your patch, but I can confirm that the MethodImplCache in the protocol function is just being used as a cache

Comment by dennis zhuang [ 08/Aug/14 6:21 AM ]

I developed a new patch that convert the methodCache in MultiFn to use WeakReference for dispatch value,and clear the cache if necessary.

I've test it with the code in ticket,and it looks fine.The classes will be unloaded when perm gen is almost all used up.

Comment by Alex Miller [ 22/Aug/14 4:55 PM ]

I don't know which to evaluate here. Does multifn_weak_method_cache.diff supersede naive-lru-for-multimethods-and-protocols.diff or are these alternate approaches both under consideration?

Comment by Kevin Downey [ 22/Aug/14 8:26 PM ]

the most straight forward thing, I think, is to consider them as alternatives, I am not a huge fan of weakrefs, but of course not using weakrefs we have to pick some bounding size for the cache, and the cache has a strong reference that could prevent a gc, so there are trade offs. My reasons to stay away from weak refs in general are using them ties the behavior of whatever you are building to the behavior of the gc pretty strongly. that may be considered a matter of personal taste

Comment by Andy Fingerhut [ 29/Aug/14 4:31 PM ]

All patches dated Aug 8 2014 and earlier no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. They did apply cleanly before that day.

I have not checked how easy or difficult it might be to update the patches.

Comment by Kevin Downey [ 29/Aug/14 7:00 PM ]

I've updated naive-lru-for-multimethods-and-protocols.diff to apply to the current master

Comment by Andy Fingerhut [ 29/Aug/14 7:34 PM ]

Thanks, Kevin. While JIRA allows multiple attachments to a ticket with the same filename but different contents, that can be confusing for people looking for a particular patch, and for a program I have that evaluates patches for things like whether they apply and build cleanly. Would you mind removing the older one, or in some other way making all the names unique?

Comment by Kevin Downey [ 29/Aug/14 8:43 PM ]

I deleted all of my attachments accept for my latest and greatest

Comment by dennis zhuang [ 30/Aug/14 9:51 AM ]

I updated multifn_weak_method_cache2.diff patch too.

I think using weak reference cache is better,because we have to keep one cache per multifn.When you have many multi-functions, there will be many LRU caches in memory,and they will consume too much memory and CPU for evictions. You can't choose a proper threshold for LRU cache in every environment.
But i don't have any benchmark data to support my opinion.

Comment by Alex Miller [ 10/Sep/14 2:38 PM ]

I'm going to set the LRU cache patch aside. I don't think it's possible to find a "correct" size for it and it seems weird to me to extend APersistentMap to build such a thing anyways.

I think it makes more sense to follow the same strategy used for other caches (such as the Keyword cache) - a combination ConcurrentHashMap with WeakReferences and a ReferenceQueue for clean-up. I don't see any compelling reason not to take the same path as other internal caches.

Comment by Alex Miller [ 10/Sep/14 3:44 PM ]

Stepping back a little to think about the problem.... our requirements are:
1) cache map of dispatch value (could be any Object) to multimethod function (IFn)
2) do we want keys to be compared based on equality or identity? identity-based opens up more reference-based caching options and is fine for most common dispatch types (Class, Keyword), but reduces (often eliminates?) cache hits for all other types where values are likely to be equiv but not identical (vector of strings for example)
3) concurrent access to cache
4) cache cannot grow without bound
5) cache cannot retain strong references to dispatch values (the cache keys) because the keys might be instances of classes that were loaded in another classloader which will prevent GC in permgen

multifn_weak_method_cache.diff uses a ConcurrentHashMap (#3) that maps RefWrapper around keys to IFn (#1). The patch uses Util.equals() (#2) for (Java) equality-based comparisons. The RefWrapper wraps them in WeakReferences to avoid #5. Cache clearing based on the ReferenceQueue is used to prevent #4.

A few things definitely need to be fixed:

  • Util.equals() should be Util.equiv()
  • methodCache and rq should be final
  • Why does RefWrapper have obj and expect rq to possibly be null?
  • RefWrapper fields should all be final
  • Whitespace errors in patch

Another idea entirely - instead of caching dispatch value, cache based on hasheq of dispatch value then equality check on value. Could then use WeakHashMap and no RefWrapper.

This patch does not cover the protocol cache. Is that just waiting for the multimethod case to look good?

Comment by dennis zhuang [ 10/Sep/14 7:18 PM ]

Hi, alex, thanks for your review.But the latest patch is multifn_weak_method_cache2.diff. I will update the patch soon by your review, but i have a few questions to be explained.

1) I will use Util.equiv() instead of Util.equals().But what's the difference of them?
2) When the RefWrapper is retained as key in ConcurrentHashMap, it wraps the obj in WeakReference.But when trying to find it in ConcurrentHashMap, it uses obj directly as strong reference, and create it with passing null ReferenceQueue.Please look at the multifn_weak_method_cache2.diff line number 112. It short, the patch stores the dispatch value as weak reference in cache,but uses strong reference for cache getting.

3) If caching dispatch value based on hasheq , can we avoid hasheq value conflicts? If two different dispatch value have a same hasheq( or why it doesn't happen?), we would be in trouble.

Sorry, the patch doesn't cover the protocol cache, i will add it ASAP.

Comment by dennis zhuang [ 11/Sep/14 2:02 AM ]

The new patch 'protocol_multifn_weak_ref_cache.diff' is uploaded.

1) Using Util.equiv() instead of Util.equals()
2) Moved the RefWrapper and it's associated methods to Util.java, and refactor the code based on alex's review.
3) Fixed whitespace errors.
4) Fixed PermGen leak in protocol fns.

Comment by Alex Miller [ 03/Oct/14 10:35 AM ]

I screened this ticket again with Brenton Ashworth and had the following comments:

1) We need to have a performance test to verify that we have not negatively impacted performance of multimethods or protocol invocation.
2) Because there are special cases around null keys in the multimethod cache, please verify that there are existing example tests using null dispatch values in the existing test coverage.
3) In Util$RefWrapper.getObj() - why does this return this.ref at the end? It was not clear to me that the comment was correct or that this was useful in any way.
4) In Util$RefWrapper.clearRefWrapCache() - can k == null in that if check? If not, can we omit that? Also, if you explicitly create the Iterator from the entry set, you can call .remove() on it more efficiently than calling .remove() on the cache itself.
5) In core_deftype / MethodImplCache, it appears that you are modifying a now-mutable field rather than the prior version that was going to great lengths to stay immutable. It's not clear to me what the implications of this change are and that concerns me. Can it use a different collection or code to stay immutable?
6) Please update the description of this ticket to include an approach section that describes the changes we are making.

Thanks!





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

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

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-1534] Adding condp-> and condp->> macros to core library Created: 24/Sep/14  Updated: 12/Jul/16

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

Type: Enhancement Priority: Major
Reporter: Kuldeep Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: enhancement, macro

Attachments: File clj_1534.diff     File condp-threading-macros-25sept2014.diff    
Patch: Code

 Description   

After introduction of cond-> and cond->> macros in 1.5. It makes sense to have condp-> and condp->> macros in the core library.

(condp-> {}
(complement :a) (assoc :a 1)
:a (assoc :b 2)) ;=> {:b 2, :a 1}

In the above example the result of each expr which was evaluated is being passed to the next predicate.



 Comments   
Comment by Andy Fingerhut [ 01/Oct/14 6:37 PM ]

Kuldeep, I cannot comment on whether this change is of interest to the Clojure developers, because I do not know.

I can say that the patch you have attached is not in the expected format. See the page below for instructions on creating a patch in the expected format:

http://dev.clojure.org/display/community/Developing+Patches

Comment by Kuldeep [ 28/Jan/15 11:31 AM ]

Rebased against master and generated patch as described in wiki.

Comment by Vitalie Spinu [ 12/Jul/16 5:21 AM ]

This is a very common pattern for me.

This is one way of dealing with such state-dependent conditionals:

(-> x
  (as-> y (if (:foo y) (assoc y :boo 0) y))
  ...)

The proposed `condp->` is much more readable:

(-> x
  (condp-> :foo (assoc :boo 0))
  ...)

BTW, `condp->` is not exactly the counterpart of `condp`. So maybe shorter `pred->` or `p->` are better names for this.





[CLJ-1775] Add any? Created: 07/Jul/15  Updated: 11/Jul/16  Resolved: 11/Jul/16

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

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


 Description   

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



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

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

Comment by Joshua Griffith [ 11/Jul/16 10:36 AM ]

Was this added in 1.9.0-alpha10?

Comment by Alex Miller [ 11/Jul/16 10:41 AM ]

Yes.

Comment by Nicola Mometto [ 11/Jul/16 2:21 PM ]

For future reference, note that the one added in 1.9.0-alpha10 is not the same `any?` that was proposed in this ticket

Comment by Alex Miller [ 11/Jul/16 3:40 PM ]

Oh, good point - I didn't read that closely enough.

Comment by Alex Miller [ 11/Jul/16 3:41 PM ]

updating ticket status to accurately reflect





[CLJ-1977] Printing a Throwable throws if Throwable has no cause / stacktrace Created: 06/Jul/16  Updated: 08/Jul/16  Resolved: 08/Jul/16

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

Type: Defect Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: regression
Environment:

alpha9


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

 Description   

Throwable->map in core_print.clj doesn't handle Throwable.getCause returning null in L463. This results in a NPE in StrackTraceElement->vec in the same file in some cases, so printing a stacktrace results in a new exception being thrown which is a bit confusing.

Repro:

(def t (Throwable.))
(.setStackTrace t (into-array StackTraceElement []))
(Throwable->map t) ;; throws npe during conversion
(pr t) ;; throws during printing

Approach: Check that at least one StackTraceElement exists before using the top frame. Make printing tolerant of a missing :at value. Add test for this omitted stack trace case.

Patch: clj-1977.patch






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

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

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

Clojure 1.6.0
Clojure 1.7.0-alpha5
Clojure 1.7.0-beta3

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


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

 Description   

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

Printing

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

Can't read back

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

Possible Fixes

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


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

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

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

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

(defrecord Rec [f])

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

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

Comment by Mike Rodriguez [ 05/Jul/16 11:29 PM ]

This won't work for sorted sets (or maps) that are defined with a custom Comparator though via fn's like sorted-set-by etc. I think the round-trip print to read result would then be confusing and incorrect right?

Even more troublesome to me here is that I see no clear way to make print-dup capable of handling the case of a custom Comparator correctly. Arbitrary functions are black boxes and we have no generally, effective way to print-dup them (based on my research I assume this to be correct). We can always make special wrapped fn's for that, but again, not general.





[CLJ-1970] instrumented macros never conform valid forms Created: 25/Jun/16  Updated: 05/Jul/16  Resolved: 05/Jul/16

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

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

Approval: Ok

 Description   

Although macros can be speced without &form and &env, once they are instrumented they will try to conform the args including &form/&env and fail:

user=> (require '[clojure.spec :as s])
nil
user=> (defmacro m [x] x)
#'user/m
user=> (s/fdef m :args (s/cat :arg integer?) :ret integer?)
user/m
user=> (m 1)
1
user=> (m a)
CompilerException java.lang.IllegalArgumentException: Call to user/m did not conform to spec:
In: [0] val: a fails at: [:args :arg] predicate: integer?
:clojure.spec/args  (a)
, compiling:(NO_SOURCE_PATH:5:1) 
user=> (s/instrument)
[user/m]
user=> (m 1)
ExceptionInfo Call to #'user/m did not conform to spec:
In: [0] val: (m 1) fails at: [:args :arg] predicate: integer?
:clojure.spec/args  ((m 1) nil 1)
  clojure.core/ex-info (core.clj:4718)
user=>

To resolve the situation, I think instrument/instrument-all should avoid speced macros.



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

This is an issue we're discussing - for the moment, you should not instrument macros. There is no point in instrumenting them as they are automatically checked during macroexpansion.

Comment by OHTA Shogo [ 25/Jun/16 10:00 AM ]

Yes, I know the compiler checks macro specs automatically, but just thought it would be nice if explicit calls to instrument (with no args) and instrument-all would check whether or not each speced var is a macro and filter it out if so.

Comment by Alex Miller [ 25/Jun/16 2:30 PM ]

Totally agreed.

Comment by Alex Miller [ 05/Jul/16 1:58 PM ]

Fixed in https://github.com/clojure/clojure/commit/d8aad06ba91827bf7373ac3f3d469817e6331322 for 1.9.0-alpha9





[CLJ-1976] using spec/fspec seems to require clojure.test.check Created: 05/Jul/16  Updated: 05/Jul/16  Resolved: 05/Jul/16

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

Type: Defect Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: spec
Environment:

clojure 1.9.0-alpha8



 Description   
(spec/def ::translate
  (spec/fspec
    :args (spec/cat :locale keyword?
                    :key keyword?
                    :args (spec/* ::spec/any))
    :ret  string?))

(defn tr [l k & args] ...)

(spec/conform ::translate tr)

Uncaught exception, not in assertion.
expected: nil
  actual: java.io.FileNotFoundException: Could not locate clojure/test/check/generators__init.class or clojure/test/check/generators.clj on classpath.


 Comments   
Comment by Alex Miller [ 05/Jul/16 10:52 AM ]

Dupe of CLJ-1936 I believe.





[CLJ-1973] generate-proxy produces unpredictable method order in generated classes Created: 01/Jul/16  Updated: 04/Jul/16

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

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: Prescreened

 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-1935] clojure.spec/multi-spec ignores the multimethod hierarchy Created: 28/May/16  Updated: 04/Jul/16

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

Type: Defect Priority: Major
Reporter: Viktor Magyari Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: multimethods, spec

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

 Description   

Minimal example:

(require '[clojure.spec :as s])

(def h (derive (make-hierarchy) :a :b))

(defmulti spec-type identity :hierarchy #'h)

(defmethod spec-type :b [_]
  (s/spec (constantly true)))

(s/def ::multi (s/multi-spec spec-type identity))

(s/explain ::multi :b)
;; => Success!
;; as expected

(s/explain ::multi :a)
;; => val: :a fails at: [:a] predicate: spec-error/spec-type,  no method
;; fails even though :a derives from :b

Also fails with the default hierarchy. Worked fine in alpha1, broken in alpha2 and alpha3.






[CLJ-1974] Clojure.org URLs in docstrings are broken Created: 03/Jul/16  Updated: 04/Jul/16  Resolved: 04/Jul/16

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

Type: Defect Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

Many links of the form http://clojure.org/data_structures#hash now have the form http://clojure.org/reference/data_structures#hash with the reference/ subpath in it.

I think the right thing to do is to set up some up some redirects on clojure.org, but if you think it's better to change the docstrings, I can submit a patch.



 Comments   
Comment by Alex Miller [ 03/Jul/16 7:00 PM ]

Hmm, I actually did set up redirects for all old links, so something must be up with the deployment. In the future, filing issues about the site is best at the Clojure-site github issues. We don't plan to change the links in the source.

Comment by Brandon Bloom [ 03/Jul/16 8:26 PM ]

Didn't realize that was on GH. For other looking, I found it: https://github.com/clojure/clojure-site

Comment by Alex Miller [ 04/Jul/16 8:32 AM ]

Last deploy of the site failed so redirects were broken. Site now redeployed and links working.

Comment by Alex Miller [ 04/Jul/16 9:18 AM ]

Deployment failure is due to intermittent AWS errors so I have also added some automatic retry support.





[CLJ-790] Primitive type hints on function names should print error message Created: 10/May/11  Updated: 02/Jul/16

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

Type: Enhancement Priority: Major
Reporter: Alan Dipert Assignee: Alan Dipert
Resolution: Unresolved Votes: 0
Labels: errormsgs


 Description   

Functions returning primitives are hinted with metadata on the argument list, not on the function name. Using a primitive type hint on a function name should print an error message.

Currently, misplaced primitive hints are read without error.



 Comments   
Comment by Andy Fingerhut [ 23/Feb/15 9:20 PM ]

One can type hint a primitive value on a Var naming a function, or any value one wants, like so:

(def {:tag 'long} foo 17)

(defn {:tag 'double} bar [x y]
(* 2.0 x y))

I think it is odd that one must use {:tag 'long} instead of ^long, since trying to use ^long ends up giving the useless type hint that is the value of the function clojure.core/long.

However, the Clojure compiler will use the primitive type hints as shown in the examples above to avoid reflection in appropriate Java interop calls, so making them an error seems undesirable.

Comment by lvh [ 02/Jul/16 10:07 AM ]

Alternatively, perhaps the compiler could simply use the type hint? While ^long is useless now, its intent seems unambiguous.





[CLJ-200] Extend cond to support inline let, much like for Created: 18/Oct/09  Updated: 01/Jul/16  Resolved: 01/Jul/16

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Mark Engelberg
Resolution: Declined Votes: 11
Labels: None

Attachments: File clj-200-cond-let-clauses-fixed-test-v2.diff     Text File clj-200-cond-let-clauses-fixed-test-v2-patch.txt    
Patch: Code and Test

 Description   

I find it occasionally very useful to do a few tests in a cond, then introduce some new symbols (for both clarity and efficiency) that can be referenced in later tests (or matching expressions). This parallels similar functionality inside the for macro, where the :let keyword is matched against a vector of symbol bindings and forms an implicit let around the remainder of the comprehension.

I'll be adding a patch for this shortly.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 1:51 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/200

Comment by Assembla Importer [ 24/Aug/10 1:51 PM ]

hlship said: Trickier than I thought because cond is really wired into other fundamentals, like let.

Comment by Assembla Importer [ 24/Aug/10 1:51 PM ]

cgrand said: Howard, what do you think of http://gist.github.com/432712 ?

Comment by Mark Engelberg [ 23/Nov/12 2:33 AM ]

Patch cond-let-clauses.diff on 23/Nov/12 adds inline :let clauses to cond, implementing CLJ-200. The code is based off of code by cgrand, with some tweaks so the implementation only relies on constructs defined earlier in core.clj, since when cond is defined, things aren't yet fully bootstrapped. Also added a test to control.clj.

Comment by Christophe Grand [ 23/Nov/12 3:06 AM ]

Some comments: the docstring is missing, I believe you don't have to modify the original cond (except the docstring maybe), just redefine it later on once most of the language is defined – a bit like what is done for let for example.

There is still the unlikely eventuality that some code uses :let as :else. What about shipping a cond which complains on keywords (in test position) other than :else?

Comment by Mark Engelberg [ 23/Nov/12 3:47 AM ]

cond-let-clauses-with-docstring.diff contains the same patches as cond-let-clauses, but includes the original docstring for cond along with an additional sentence about the :let bindings.

Comment by Mark Engelberg [ 23/Nov/12 3:54 AM ]

Cgrand, I did see your example of redefining cond after most of the language is defined, but since I was able to figure out how to do it in the proper place, that makes the :let bindings available for users of cond downstream and avoids any unforeseen complications that might come from rebinding.

As for your other point, I think it is highly improbable that someone would have used :let in the :else position. However I can imagine someone intentionally using something like :true or :default. I think the idea of warning for other keywords is actually more likely to cause complications than the unlikely problem it is meant to solve.

I did resubmit the patch with the docstring restored. Thanks for pointing out that problem. I'm excited about this patch – I use :let bindings within the cond in my own code all the time. Thanks again for the blog post that started me on that path.

Comment by Christophe Grand [ 23/Nov/12 4:13 AM ]

True, it's :unlikely for :let to happen.
However once :let is officially blessed, it may be better to provision for future other "special" keywords and thus to warn on "unsupported" keywords. Plus it will help out-of-order typists (like myself) to catch earlier a :elt instead of a :let
This is only my point of view. Thanks for trying to get :let in cond supported.

Comment by Andy Fingerhut [ 29/Nov/12 8:46 PM ]

Mark, could you remove the obsolete earlier patch now that you have added the one with the doc string? Instructions for removing patches are under the heading "Removing Patches" on this page: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Mark Engelberg [ 29/Nov/12 10:50 PM ]

Done.

Comment by Andy Fingerhut [ 30/Nov/12 1:24 AM ]

I haven't figured out what is going wrong yet. I can apply the patch cond-let-clauses-with-docstring.diff to the latest Clojure master just fine. I can do "ant jar" and it will build a jar. When I do "ant", it fails with the new test for cond with :let, throwing a StackOverflowException. I can enter that same form into the REPL and it evaluates just as the test says it should. I can comment out that new test and all of the rest pass. But the new test doesn't pass when inside of the control.clj file. Anyone know why?

Comment by Christophe Grand [ 30/Nov/12 4:54 AM ]

It's because of the brutal replacement performed by test/are: the placeholders for this are form are x and y but in Mark's test there are used as local names and are tries to substitute them recursively...
If one changes the local names to a and b for example it works.

Comment by Mark Engelberg [ 02/Dec/12 8:20 AM ]

cond-let-clauses-fixed-test.diff on 02/Dec/12 contains the same patch, but with the x,y locals in the test case changed to a,b so that it works properly in the are clause which uses x and y.

Comment by Mark Engelberg [ 02/Dec/12 8:27 AM ]

On Windows, I can't get Clojure's test suite task to work, either via ant or maven, which has made it difficult for me to verify the part of the patch that applies to the test suite works as expected; I had tested it as best I could in the REPL, using a version of Clojure built with the patch applied, but using this process, I missed the subtle interaction between are and the locals in the test case. Sorry about that. If someone can double-check that the test suite task now works with the newest patch, that would be great, and then I'll go ahead and remove the obsoleted patch. Thanks.

Comment by Andy Fingerhut [ 02/Dec/12 6:29 PM ]

clj-200-cond-let-clauses-fixed-test-v2-patch.txt dated Dec 2 2012 is identical to Mark Engelberg's cond-let-clauses-fixed-test.diff of the same date, except it applies cleanly to the latest Clojure master.

I've verified that it compiles and passes all tests with latest Clojure master as of this date.

Mark, I've made sure to keep your name in the patch, since you wrote it. You should be able to remove your two attachments now, so the screener won't be confused which patch should be examined.

Comment by Andy Fingerhut [ 02/Dec/12 6:31 PM ]

Mark, besides general issues with Windows not being used much (or maybe not at all?) by Clojure developers, there is the issue right now filed as CLJ-1076 that not all tests pass when run on Windows due to CR-LF line ending differences that cause several Clojure tests to fail, regardless of whether you use ant or maven to run them.

Comment by Andy Fingerhut [ 22/Oct/13 8:01 PM ]

clj-200-cond-let-clauses-fixed-test-v2.diff is identical to earlier patch clj-200-cond-let-clauses-fixed-test-v2-patch.txt, except it removes unnecessarily trailing whitespace that causes warnings when applying the patch, and with .diff suffix for easier diff-style viewing in some editors.

Comment by Mark Engelberg [ 01/Jul/16 7:09 PM ]

It was just brought to my attention that this issue was declined today.

FWIW, earlier today I wrote up a Rationale about this issue as part of my effort to address it temporarily at the library level. I hope you will take a look:

https://github.com/Engelberg/better-cond#rationale





[CLJ-993] `range` reducer Created: 10/May/12  Updated: 29/Jun/16

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

Type: Enhancement Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reducers

Attachments: Text File 0001-CLJ-993-implement-range-and-iterate-as-reducers.patch     Text File 0001-CLJ-993-implement-range-and-iterate-as-reducers.patch     Text File 0001-CLJ-993-implement-range-as-a-reducer.patch     Text File 0002-Make-iterate-and-range-Seqable.patch     Text File 0003-Implement-fold-for-Range-objects.patch     Text File just-iseq.patch     Text File range-reducer.patch    
Patch: Code and Test

 Description   

Rich mentioned in IRC today he'd welcome a reducer implementation of clojure.core/range. Now that I've figured out how to do iterate, I figure I'll knock out range as well by the end of the night. Just opening the issue early to announce my intentions to anyone else interested in doing it.



 Comments   
Comment by Alan Malloy [ 10/May/12 10:45 PM ]

Implemented range. A separate commit is attached, making iterate and range also Seqable, since I'm not sure if that's desired. Apply it or not, as you prefer.

Comment by Jason Jackson [ 11/May/12 11:20 AM ]

Range should be foldable

Comment by Alan Malloy [ 11/May/12 12:53 PM ]

Yep, so it should. Time for me to dig into the folding implementations!

Comment by Alan Malloy [ 11/May/12 2:42 PM ]

Should I fold (har har) all of these commits into one? I don't know what is preferred on JIRA, and I also don't know whether range/iterate should be seqable or if I should just drop the second commit.

Comment by Rich Hickey [ 11/May/12 3:21 PM ]

Yes, please merge these together, it's hard to see otherwise (I can barely read diffs as is . range and iterate shouldn't be novel in reducers, but just enhanced return values of core fns. The enhancement (e.g. protocol extensions) can come by requiring reducers since it can't be leveraged without it. Also, I'm not sure how I feel about an allocating protocol for 'splittable' - I've avoided it thus far.

Comment by Alan Malloy [ 11/May/12 3:30 PM ]

So you want clojure.core/range to return some object (a Range), which implements Counted and Seqable (but isn't just a lazy-seq), and then inside of clojure.core.reducers I extend CollReduce and CollFold to that type? Okay, I can do that.

I don't quite follow what you mean by an allocating protocol. I see your point that my fold-by-halves which takes a function in is analogous to a protocol with a single function, but it doesn't allocate anything more than foldvec already does - I just pulled that logic out so that the fork/join fiddly work doesn't need to be repeated in everything foldable. Do you have an alternative recommendation, or is it just something that makes you uneasy and you're still thinking about?

Comment by Rich Hickey [ 11/May/12 3:52 PM ]

While vector-fold allocs subvecs, the halving-fn must return a new vector, for all implementations. It's ok, I don't think it's likely to dominate (since fj needs new closures anyway). Please proceed, but keep range and iterate in core. They are sources, not transformers, and only transformers (which must be different from their seq-based counterparts) must reside in reducers. Thanks!

Comment by Stuart Sierra [ 11/May/12 5:01 PM ]

One big patch file is preferred, although that file may contain multiple commits if that makes the intent clearer.

When adding a patch, update the description of the ticket to indicate which file is the most recent. Leave old patch files around for historical reference.

Comment by Alan Malloy [ 11/May/12 9:00 PM ]

It's looking harder than I expected to move iterate and range into core.clj. My plan was to just have them implement Seqable, which is easy enough, but currently they are actually instances of ISeq, because they inherit from LazySeq. A bunch of code all over the place (eg, to print them in the repl) depends on them being ISeq, so I can't just ignore it. To implement all of these methods (around thirty) would take a large amount of code, which can't easily be shared between Iteration, Range, and any future reducible sources that are added to core.clj.

I could write a macro like (defseq Range [start end step] Counted (count [this] ...) ...) which takes normal deftype args and also adds in implementations for ISeq, Collection, and so forth in terms of (.seq this), which will be a LazySeq. However, this seems like a somewhat awkward approach that I would be a little embarrassed to clutter up core.clj with. If anyone has a better alternative I will be pleased to hear it. In the mean time, I will go ahead with this macro implementation, in case it turns out to be the best choice.

Comment by Alan Malloy [ 11/May/12 11:52 PM ]

– This patch subsumes all previous patches to this issue and to CLJ-992

In order to create an object which is both a lazy sequence and a
reducible source, I needed to add a macro named defseq to core_deftype.
It is basically a reimplementation of clojure.lang.LazySeq as a clojure
macro, so that I can "mix in" lazy-sequence functions into a new class
with whatever methods are needed for reducing and folding.

If we wanted, we could use this macro to implement lazy-seq in clojure instead of in java, but that's unrelated so I didn't do that in this patch.

As noted in a previous comment, defseq may not be the right approach, but this works until something better is suggested.

Comment by Alan Malloy [ 11/May/12 11:58 PM ]

I accidentally included an implementation of drop-while in this patch, which I was playing around with to make sure I understood how this all works. I guess I'll leave it in for the moment, since it works and is useful, but I can remove it, or move it to a new JIRA ticket, if it's not wanted at this time.

Comment by Rich Hickey [ 12/May/12 10:52 AM ]

Ok, I think this patch is officially off the rails. There must be a better way. Let's start with: touching core/deftype and reimplementing lazy-seq as a macro are off the table. The return value of range doesn't have to be a LazySeq, it has to be a lazy seq, .e.g. implement ISeq (7 methods, not 30) which it can do by farming out to its existing impl. It can also implement some new interface for use by the reducer logic. There is also still clojure.lang.Range still there, which is another approach. Please take an extremely conservative approach in these things.

Comment by Alan Malloy [ 12/May/12 5:53 PM ]

Okay, thanks for the feedback - I'm glad I went into that last patch knowing it was probably wrong . I thought I would need to implement the java collection interfaces that LazySeq does, eg java.util.List, in order to avoid breaking interop functions like (defn range-list [n] (ArrayList. (range n))). If it's sufficient to implement ISeq (and thus IPersistentCollection), then that's pretty manageable.

It's still an unpleasant chunk of boilerplate for each new source, though; would you welcome a macro like defseq if I didn't put it in core_deftype? If so, it seems like it might as well implement the interop interfaces; if not, I can skip them and implement the 7 (isn't it more like 9?) methods in ISeq, IPersistentCollection, and Seqable for each new source type.

Thanks for pointing out clojure.lang.Range to me - I didn't realize we had it there. Of course with implementation inheritance it would be easy to make Range, Iteration, etc inherit from LazySeq and just extend protocols from them. But that means moving functionality out of clojure and into java, which I didn't think we'd want to do.

I'll put together a patch that just implements ISeq by hand for both of these new types, and attach it probably later today.

Comment by Alan Malloy [ 12/May/12 7:49 PM ]

So I've written a patch that implements ISeq, but not the java Collections interfaces, and it mostly works but there are definitely assumptions in some parts of clojure.core and clojure.lang that assume seqs are Collections. The most obvious to me (ie, it shows up when running mvn test) is RT/toArray - it tests for Collection, but never for ISeq, implying that it's not willing to handle an ISeq that is not also a collection. Functions which rely on toArray (eg to-array and vec) now fail.

This patch subsumes all previous patches on this issue, but is not suitable for application because it leaves some failing tests behind - it is intended only for intermediate feedback.

Comment by Rich Hickey [ 13/May/12 8:50 AM ]

It would be a great help if, time permitting, you could please write up the issues, challenges and options you've discovered somewhere on the dev wiki (even a simple table would be fantastic). I realize this has been a challenging task, and at this point perhaps we should opt for the more modest reducers/range and reducers/iterate and leave the two worlds separate. I'd like at some point to unify range, as there are many extant ranges it would be nice to be able to fold, as we can extant vectors.

Comment by Jason Jackson [ 13/May/12 9:24 AM ]

Should r/range return something Seqable and Counted?

If so, I'll do the same for r/repeat.

Comment by Alan Malloy [ 13/May/12 1:59 PM ]

I've sketched out a description of the issues and options. I'm not very familiar with the dev wiki and couldn't figure out where was the right place to put this. "release.next" seems to still be about 1.4 issues, and I don't know if it's "appropriate" to create a whole new category for this. It's available as a gist until a better home can be found for it.

Comment by Alan Malloy [ 23/May/12 7:54 PM ]

Here's a single patch summing up the state Rich suggested "rolling back" to: separate r/range and r/iterate functions. I haven't heard any feedback since doing the writeup Rich asked for, so am not making any further progress at the moment; if something other than this patch is desired just let me know.

Comment by Rich Hickey [ 14/Aug/12 2:07 PM ]

I prefer not to see the use of extend like this for new types. Perhaps this code is too DRY? Also, it does a lot in one patch which makes it hard to parse and accept. This adds Range, switches impl of vector folds etc. Can it be broken up into separate tickets that do each step that builds on the previous, e.g. one ticket could be: capture vector fold impl for reuse by similar things.

Comment by Alan Malloy [ 18/Aug/12 6:19 PM ]

Okay, I should be able to split it up over the weekend. I'll also see about converting fold-by-halves into a function that is used by Range/Vector, rather than a function that gets extended onto them.

Comment by Alan Malloy [ 18/Aug/12 7:18 PM ]

As requested, I have split up the large patch on this issue into four smaller tickets. The other three are: CLJ-1045, CLJ-1046, and CLJ-992.

CLJ-1045 contains the implementation of fold-by-halves, and as such this patch cannot be applied until CLJ-1045 is accepted. This ticket does not depend on the other two, but there will be minor merge conflicts if this is merged before them.

Comment by Ghadi Shayban [ 09/Jan/16 5:31 PM ]

range is now reducible.

Comment by Ghadi Shayban [ 29/Jun/16 9:17 PM ]

We should close this too. The only thing not covered in 1.7 is the foldable aspect.





[CLJ-994] repeat reducer Created: 11/May/12  Updated: 29/Jun/16

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

Type: Enhancement Priority: Major
Reporter: Jason Jackson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reducers

Attachments: Text File 0001-repeat-for-clojure.core.reducers.patch    
Patch: Code and Test

 Description   

i'm working on clojure.core/repeat reducer.



 Comments   
Comment by Andy Fingerhut [ 17/May/12 6:18 PM ]

Jason, have you tried to build this using JDK 1.6.0? I've tried on Mac OS X 10.6.8 + Oracle/Apple JDK 1.6.0 and Ubuntu 11.10 + IBM JDK 1.6.0, and on both it compiles, but during the tests fails with a ClassNotFoundException for class jsr166y.ForkJoinTask.

It builds and tests cleanly on Ubuntu 11.10 + Oracle JDK 1.7.0 for me.

Comment by Jason Jackson [ 17/May/12 6:41 PM ]

That's an issue that applies to all of core.reducers. Alan Malloy experienced it as well. I tried fixing it, but eventually just upgraded to JDK 1.7. I don't understand why it's happening.

Comment by Jason Jackson [ 19/May/12 2:55 PM ]

This issue is isolated to mvn test afaik.

When I include clojure inside a leiningen project, and add jsr166y.jar to lib directory, core.reducers works fine with java 1.6.

Comment by Andy Fingerhut [ 20/May/12 3:00 AM ]

Jason, you say it applies to all of core.reducers in your May 17, 2012 comment. I don't understand. Without your patch applied, I can run "./antsetup.sh ; ant" in a freshly-pulled Clojure git repo on either of the JDK 1.6.0 versions mentioned in my earlier comment, and do not get any errors during the tests. Are you saying perhaps that core.reducers currently has no tests that exercise the problem now, but your patch adds such tests that fail, even with no other changes to the code?

Comment by Jason Jackson [ 20/May/12 11:55 AM ]

Yah that's right. Now that you mention it, my patch is the first unit test to call r/fold (the existing tests do non-parallel reductions).

Comment by Andy Fingerhut [ 08/Jun/12 7:11 PM ]

With Stuart Halloway's commit to Clojure master on June 8, 2012 titled "let reducers tests work under ant", patch 0001-repeat-for-clojure.core.reducers.patch dated May 11, 2012 now runs correctly even the new unit tests requiring class jsr166y.ForkJoinTask with Oracle/Apple JDK 1.6 and Linux IBM JDK 1.6.

Comment by Jason Jackson [ 14/Aug/12 1:17 AM ]

I'm on the contributors list. Is this patch still needed?
sorry for long long delay.

Comment by Jason Jackson [ 14/Sep/12 2:37 PM ]

This patch should wait until http://dev.clojure.org/jira/browse/CLJ-993 is committed. I think there's a some shared code.

Comment by Ghadi Shayban [ 09/Jan/16 5:24 PM ]

repeat is now reducible in 1.7.0

Comment by Ghadi Shayban [ 29/Jun/16 9:05 PM ]

Can we close this ticket?





[CLJ-1523] Add 'doseq' like macro for transducers Created: 08/Sep/14  Updated: 29/Jun/16

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

Type: Enhancement Priority: Trivial
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: File doreduced2.diff     File doreduced.diff    
Patch: Code and Test
Approval: Triaged

 Description   

Doseq is currently a good way to execute a lazy sequence and perform side-effects. It would be nice to have a matching macro for transducers.

Approach: The included patch simply calls transduce with the provided xform, collection, and a reducing function that throws away the accumulated value at each step. The value from each reducing step is bound to the provided symbol. A shorter arity is provided for those cases when no xform is desired, but fast doseq-like semantics are still wanted.

Patch: doreduced2.diff



 Comments   
Comment by Jozef Wagner [ 09/Sep/14 4:19 AM ]

How about making xform parameter optional? And you have a typo in docstring example, doseq -> doreduced.

Comment by Timothy Baldridge [ 09/Sep/14 7:52 AM ]

Good point, fixed typeo, added other arity.

Comment by Ghadi Shayban [ 29/Jun/16 9:03 PM ]

perhaps another arity on `run!`





[CLJ-1451] Add take-until Created: 20/Jun/14  Updated: 29/Jun/16

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

Type: Enhancement Priority: Critical
Reporter: Alexander Taggart Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: transducers

Attachments: Text File 0001-CLJ-1451-add-take-until.patch     Text File 0002-CLJ-1451-add-drop-until.patch     Text File 0003-let-take-until-and-drop-until-return-transducers.patch     Text File CLJ-1451-drop-until.patch     Text File clj-1451.patch     Text File CLJ-1451-take-until.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

Discussion: https://groups.google.com/d/topic/clojure-dev/NaAuBz6SpkY/discussion

It comes up when I would otherwise use (take-while pred coll), but I need to include the first item for which (pred item) is false.

(take-while pos? [1 2 0 3]) => (1 2)
(take-until zero? [1 2 0 3]) => (1 2 0)

Patch: clj-1451.patch

  • Includes transducer arity of take-until
  • Includes inclusion in transducer generative tests

Prescreened by: Alex Miller



 Comments   
Comment by Alex Miller [ 20/Jun/14 10:21 AM ]

Patch welcome (w/tests).

Comment by Alexander Taggart [ 20/Jun/14 2:00 PM ]

Impl and tests for take-until and drop-until, one patch for each.

Comment by Jozef Wagner [ 20/Jun/14 3:01 PM ]

Please change :added metadata to "1.7".

Comment by Alexander Taggart [ 20/Jun/14 3:12 PM ]

Updated to :added "1.7"

Comment by John Mastro [ 21/Jun/14 6:26 PM ]

I'd like to propose take-through and drop-through as alternative names. I think "through" communicates more clearly how these differ from take-while and drop-while.

Comment by Andy Fingerhut [ 06/Aug/14 2:27 PM ]

Both patches CLJ-1451-drop-until.patch and CLJ-1451-take-until.patch dated Jun 20 2014 no longer apply cleanly to latest Clojure master due to some changes committed earlier today. I haven't checked whether they are straightforward to update, but would guess that they merely require updating a few lines of diff context.

See the section "Updating stale patches" at http://dev.clojure.org/display/community/Developing+Patches for suggestions on how to update patches.

Comment by Ghadi Shayban [ 13/Nov/14 11:19 PM ]

Would be nice to cover the transducer case too.

Comment by Michael Blume [ 13/Nov/14 11:54 PM ]

rerolled patches

Comment by Michael Blume [ 14/Nov/14 12:11 AM ]

Covered transducer case =)

Comment by Michael Blume [ 14/Nov/14 12:12 AM ]

Actually I like take/drop-through as well

Comment by Ghadi Shayban [ 16/Nov/14 12:41 PM ]

Michael, no volatile/state is necessary in the transducer, like take-while. Just wrap in 'reduced to terminate

Comment by Michael Blume [ 17/Dec/14 6:47 PM ]

a) you're clearly right about take-until

b) seriously I don't know what I was thinking with my take-until implementation, I'm going to claim lack of sleep.

c) I'm confused about how to make drop-until work without a volatile

Comment by Michael Blume [ 18/Dec/14 1:52 AM ]

Ghadi and I discussed this and couldn't think of a use case for drop-until. Are there any?

Here's a new take-until patch, generative tests included.

Open questions:

Is take-until a good name? My biggest concern is that take-until makes it sound like a slight modification of take, but this function reverses the sense of the predicate relative to take.

Comment by Andy Fingerhut [ 08/Jan/15 6:06 PM ]

Michael, while JIRA can handle multiple attachments for the same ticket with the same name, it can get confusing for people trying to determine which one with the same name is meant. Could you remove or rename one of your identically-named attachments? Instructions for deleting patches are in the "Removing patches" section on this wiki page: http://dev.clojure.org/display/community/Developing+Patches

Comment by Alex Miller [ 11/Mar/16 2:49 PM ]

The patch was slightly stale so I updated to apply to master, but it's almost identical. Attribution retained.

Marked as prescreened.

Comment by Ghadi Shayban [ 29/Jun/16 9:01 PM ]

I feel like this is superceded by CLJ-1906





[CLJ-1964] rmap / *recursion-limit* not respected through custom generators Created: 19/Jun/16  Updated: 29/Jun/16  Resolved: 28/Jun/16

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

Type: Defect Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: generator, spec

Approval: Triaged

 Description   

In all cases where a custom generator is used, the recursion limit is not respected.

This limitation becomes clear when one tries to build a recursive spec around e. g. s/coll-of because it uses a custom generator via s/coll-gen. Running s/exercise on it quickly blows the stack.

Here is an example for illustration with s/map-of

(s/def ::map-tree (s/map-of keyword? (s/or :tree ::map-tree :leaf nil?)))

Even though s/or implements recursion checking, it is deceived here and not able to detect itself being called subsequently because the custom generator of s/coll-of (used in s/map-of) doesn't/can't pass rmap (keeping track of recursion calls) through to s/or's gen*.

For the concrete case, coll-of-impl could be implemented that would implement a gen* that passes on rmap.

For the general case of custom generators the challenge would be that test.check generators don't take and pass on rmap to generators of specs they potentially reuse and that there is no well-defined behavior for what they themselves should do when the recursion-limit has been reached. Ideas are:

  • reduce generator size when recursion is detected (this is the strategy used in recursive specs of test.check use(d)?)
  • expose the recursion-limit / rmap mechanism to the user so that custom generators can pass it on to subsequent calls of specs. E. g. a custom generator is passed a context object that it should pass to s/gen as an optional argument


 Comments   
Comment by Leon Grapenthin [ 19/Jun/16 5:08 PM ]

I have changed the issue because in the former description I had made some assumptions that I could prove incorrect by studying the implementation a bit more.

Comment by Alex Miller [ 25/Jun/16 9:37 AM ]

Please re-check this after the next alpha - there are a lot of changes happening in this area.

Comment by Alex Miller [ 28/Jun/16 9:58 PM ]

As of Clojure 1.9.0-alpha8, due to changes in map-of etc, s/exercise now works on this example.

Comment by Leon Grapenthin [ 29/Jun/16 9:15 AM ]

@Alex Miller - I haven't had time yet to check whether latest design changes especially in spec.test solve recursion through custom generators or make it obsolete. The examples given in above description have clearly been solved but they were only examples for a larger problem. Would you like me to change this ticket or should I create a new one?

Comment by Alex Miller [ 29/Jun/16 1:58 PM ]

I would create a new ticket unless it's substantially similar to this one, in which case you can re-open it.





[CLJ-1946] improve error messages for `map-of` spec Created: 04/Jun/16  Updated: 29/Jun/16  Resolved: 28/Jun/16

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

Type: Enhancement Priority: Major
Reporter: Chris Price Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: errormsgs, spec


 Description   

When using a map-of spec where the value predicate refers to another spec, the error message if a value does not conform does not seem like it is as helpful as it could be:

(spec/def ::myint integer?)
(spec/explain
 (spec/map-of string? ::myint)
 {"x" 1 "y" "not an int!"})
=> :user.swagger-ui-service/myint
val: {"x" 1, "y" "not an int!"} fails predicate: (coll-checker (tuple string? :user.swagger-ui-service/myint))

The explain result doesn't indicate which key/value pair in the map failed to conform, and it also doesn't make it clear that the integer? predicate is ultimately the one that caused the failure.

From reading through the introduction and docs, it seemed like error messaging was a primary motivation for spec, so any improvements that might be possible to this error message would be extremely valuable.



 Comments   
Comment by Chris Price [ 04/Jun/16 11:22 AM ]

This becomes particularly important with deeply-nested specs and/or large maps.

Comment by Sean Corfield [ 04/Jun/16 11:53 PM ]

In my opinion, the failure should use the spec name, not the underlying predicate function, in the message – isn't that the whole point of using named specs in this?

(as for explaining the value that failed, I agree on the surface but haven't looked at the implementation in detail to see if there might be a good reason)

Comment by Chris Price [ 06/Jun/16 12:23 PM ]

If nothing else, it's inconsistent with what gets logged for other types of specs:

(spec/explain
 (spec/cat :myint ::myint)
 ["hi"])
 
In: [0] val: "hi" fails spec: :puppetlabs.trapperkeeper.main/myint at: [:myint] predicate: integer?
=> nil
(spec/explain
 (spec/tuple ::myint)
 ["hi"])
 
In: [0] val: "hi" fails spec: :puppetlabs.trapperkeeper.main/myint at: [0] predicate: integer?
=> nil

Having spent a good chunk of a day hacking on a project that involved some very deeply-nested specs, I can definitively say that the logging from cat and tuple was much easier to debug. With those, I could keep poking at my "production" code via the REPL, tweaking things until the specs were correct. With map-of, the only way I was able to debug was to copy/paste the entire failed val into the REPL and cobble together a one-off spec/explain form to repro, and then delete things from it until I got past the map-of so that the error was less opaque. Then once I fixed the actual issue I'd need to copy/paste the REPL stuff back into my "production" code. It wasn't impossible, but it was a much more tedious workflow than when dealing with errors from cat or tuple.

Comment by Alex Miller [ 13/Jun/16 3:50 PM ]

The issues here is due to the way map-of and coll-of sample their contents rather than fully conforming all of them. This is done for performance but is not what most people expect. It's likely there will be some more additions in this area.

Comment by Alex Miller [ 28/Jun/16 10:00 PM ]

As of 1.9.0-alpha8, map-of now conforms all entries and the error you'll see is:

In: ["y" 1] val: "not an int!" fails spec: :user/myint at: [1] predicate: integer?
Comment by Chris Price [ 29/Jun/16 12:12 PM ]

\o/ thanks!





[CLJ-1947] Add vec-of spec Created: 05/Jun/16  Updated: 28/Jun/16  Resolved: 28/Jun/16

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

Type: Enhancement Priority: Minor
Reporter: Maarten Truyens Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: spec


 Description   

It would be great to add a "vec-of" (and perhaps also a "set-of") Spec, similar to the already existing map-of. I find myself writing (coll-of ::foo []) writing too often.



 Comments   
Comment by Alex Miller [ 28/Jun/16 10:01 PM ]

With 1.9.0-alpha8, you can now get this same effect using:

(s/coll-of ::foo :kind vector?)
(s/coll-of ::foo :kind set?)




[CLJ-1963] clojure.spec/map-of has confusing error message when input is not a map Created: 15/Jun/16  Updated: 28/Jun/16  Resolved: 28/Jun/16

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

Type: Defect Priority: Major
Reporter: Russell Mull Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: errormsgs, spec


 Description   

When using map-of specs, the error message given when checking a non-map value is less than enlightening:

user> (require '[clojure.spec :as s])
nil
user> (s/def ::int-map (s/map-of integer? integer?))
:user/int-map
user> (s/explain ::int-map :not-a-map)
val: :not-a-map fails spec: :user/int-map predicate: (coll-checker (tuple integer? integer?))
nil

This can be worked around to some degree by requiring that the value be a map explicitly:

user> (s/def ::fancy-int-map (s/and map? ::int-map))
:user/fancy-int-map
user> (s/explain ::fancy-int-map :not-a-map)
val: :not-a-map fails spec: :user/fancy-int-map predicate: map?
nil

The definition

map-of
looks like it's trying to do just this, but the
map?
predicate comes second for some reason.



 Comments   
Comment by Alex Miller [ 28/Jun/16 9:54 PM ]

map-of implementation changed a lot in alpha8 and you will now see the error for your first example as:

user=> (s/explain ::int-map :not-a-map)
val: :not-a-map fails spec: :user/int-map predicate: map?




[CLJ-1971] Update docstring of empty? to suggest not-empty instead of seq Created: 27/Jun/16  Updated: 28/Jun/16  Resolved: 28/Jun/16

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

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


 Description   

The docstring for empty? says

clojure.core/empty? [coll]
Returns true if coll has no items - same as (not (seq coll)). Please use the idiom (seq x) rather than (not (empty? x))

Would it make more sense to suggest using not-empty, instead of seq here?



 Comments   
Comment by Alex Miller [ 28/Jun/16 9:20 AM ]

No, the recommended idiom is still to use seq as a termination condition in this case.





[CLJ-1972] issue with browse-url Created: 28/Jun/16  Updated: 28/Jun/16

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

Type: Defect Priority: Trivial
Reporter: David Siefert Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Check-for-zero-exit-code-to-consider-that-script-exe.patch     Text File 0002-Extracting-method-open-url-by-script-in-browse-url.patch     Text File 0003-Extracting-explaining-method-success-in-open-url-by-.patch    
Patch: Code
Approval: Triaged

 Description   

When xdg-utils are installed on my platform, and the xdg-open command fails, (clojure.java.browse/browse-url) ignores this error and silently fails. This fix will allow the (or ..) logic to continue evaluating to try the next method.






[CLJ-1965] clojure.spec/def should support an optional doc-string Created: 19/Jun/16  Updated: 25/Jun/16

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

Type: Enhancement Priority: Minor
Reporter: Alexander Kiel Assignee: Unassigned
Resolution: Unresolved Votes: 10
Labels: spec


 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.






[CLJ-1969] :as form is unbound when no optional keyword arguments is passed even though :or form is provided Created: 24/Jun/16  Updated: 24/Jun/16  Resolved: 24/Jun/16

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

Type: Defect Priority: Trivial
Reporter: Lars Andersen Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   
(defn fn-with-kw-opts [& {:keys [opt] :or {opt 1} :as options}]
  (println "opt " opt "options " options))

user> (fn-with-kw-opts)
;;=> opt  1 options  nil

user> (fn-with-kw-opts :opt 2)
;;=> opt  2 options  {:opt 2}

I would expect options to be bound to the default value when no keyword argument is passed.



 Comments   
Comment by Alex Miller [ 24/Jun/16 7:36 AM ]

:as binds to the original value passed in and will never include values from :or. :or is used to provide defaults for each local being bound when that local is missing in the input.

In the case of (fn-with-kw-opts), the incoming value is nil so options is bound to nil.

Comment by Alex Miller [ 24/Jun/16 7:38 AM ]

Working as designed.





[CLJ-1968] clojure.test/report :error does not flush *out* when the test fails with an exception Created: 23/Jun/16  Updated: 23/Jun/16

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

Type: Defect Priority: Major
Reporter: Sam Roberton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: clojure.test

Approval: Triaged

 Description   

Minimal reproduction:

(require 'clojure.test)

(clojure.test/deftest foo-test
  (throw (ex-info "I fail" {})))

(clojure.test/deftest bar-test
  (.println System/out "bar"))

(clojure.test/test-vars [#'foo-test #'bar-test])

Result:

ERROR in (foo-test) (core.clj:4617)
Uncaught exception, not in assertion.
expected: nil
bar
  actual: clojure.lang.ExceptionInfo: I fail
 at clojure.core$ex_info.invokeStatic (core.clj:4617)
...

Note "bar" appearing in the output in the middle of the error report for foo-test.

Analysis:

(clojure.test/report {:type :error, :actual some-exception}) calls stack/print-cause-trace. Unlike other clojure.test/report callpaths, this does not flush on newline. Thus, when tests fail with exceptions and there is anything writing directly to Java's System.out, there can be a large gap between the first part of the error report and the exception trace.

(To explain why this is annoying: we're running Selenium tests via clj-webdriver, and our system under test is logging with log4j via clojure.tools.logging. We invariably see dozens or even hundreds of lines between "expected: ..." and the subsequent "actual: ..." exception trace. This makes it very easy to come to completely the wrong conclusion about when failures occurred with respect to the other events that appear interleaved in the log.)

It would be preferable (in my opinion) if clojure.test/report always constructed the output from each individual invocation into a single string which got written to *out* all at once – that way there could be no way for output to be interleaved from other threads. Absent that, it would at least help a lot if the :error implementation called (flush).






[CLJ-1919] Destructuring support for namespaced keys and syms Created: 27/Apr/16  Updated: 23/Jun/16  Resolved: 23/Jun/16

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 1
Labels: destructuring

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

 Description   

Expand destructuring to better support a set of keys (or syms) from a map when the keys share the same namespace.

Example:

(def m {:person/first "Darth" :person/last "Vader" :person/email "darth@death.star"})

(let [{:keys [person/first person/last person/email]} m]
  (format "%s %s - %s" first last email))

Proposed: The special :keys and :syms keywords used in associative destructuring may now have a namespace (eg :person/keys). That namespace will be applied during lookup to all listed keys or syms when they are retrieved from the input map.

Example (also uses the new literal syntax for namespaced maps from CLJ-1910):

(def m #:person{:first "Darth" :last "Vader" :email "darth@death.star"})

(let [{:person/keys [first last email]} m]
  (format "%s %s - %s" first last email))
  • The key list after :ns/keys should contain either non-namespaced symbols or non-namespaced keywords. Symbols are preferred.
  • The key list after :ns/syms should contain non-namespaced symbols.
  • As :ns/keys and :ns/syms are read as normal keywords, auto-resolved keywords work as well: ::keys, ::alias/keys, etc.
  • Clarification - the :or defaults map always uses non-namespaced symbols as keys - that is, they are always the same as the locals being created (not the keys being looked up in the map). No change in behavior here, just trying to be explicit - this was not previously well-documented for namespaced key lookup and was broken. The attached patch fixes this behavior.

Patch: clj-1919-2.patch



 Comments   
Comment by Alex Miller [ 06/Jun/16 7:26 AM ]

This patch now needs to be re-worked on top of https://github.com/clojure/clojure/commit/0aa346766c4b065728cde9f9fcb4b2276a6fe7b5

Comment by Alex Miller [ 14/Jun/16 9:34 AM ]

Rebased patch to current master. No semantic changes as they didn't actually conflict, just were close enough to confuse git.





[CLJ-1910] Namespaced maps Created: 07/Apr/16  Updated: 23/Jun/16  Resolved: 23/Jun/16

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 1
Labels: print, reader

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

 Description   

A common usage of namespaced keywords and symbols is in providing attribute disambiguation in map contexts:

{:person/first "Han" :person/last "Solo" :person/ship 
  {:ship/name "Millenium Falcon" :ship/model "YT-1300f light freighter"}}

The namespaces provide value (disambiguation) but have the downside of being repetitive and verbose.

Namespaced maps are a reader (and printer) feature to specify a namespace context for a map.

  • Namespaced maps combine a default namespace with a map and yield a map.
  • Namespaced maps are reader macros starting with #: or #::, followed by a normal map definition.
    • #:sym indicates that sym is the default namespace for the map to follow.
    • #:: indicates that the default namespace auto-resolves to the current namespace.
    • #::sym indicates that sym should be auto-resolved using the current namespace's aliases OR any fully-qualified loaded namespace.
      • These rules match the rules for auto-resolved keywords.
  • A namespaced map is read with the following differences from normal maps:
    • A keyword or symbol key without a namespace is read with the default namespace as its namespace.
    • Keys that are not symbols or keywords are not affected.
    • Keys that specify an explicit namespace are not affected EXCEPT the special namespace _, which is read with NO namespace. This allows the specification of bare keys in a namespaced map.
    • Values are not affected.
    • Nested map keys are not affected.
  • The edn reader supports #: but not #:: with the same rules as above.
  • Maps will be printed in namespaced map form only when:
    • All map keys are keywords or symbols
    • All map keys are namespaced
    • All map keys have the same namespace

Examples:

;; same as above - notice you can nest #: maps and this is a case where the printer roundtrips
user=> #:person{:first "Han" :last "Solo" :ship #:ship{:name "Millenium Falcon" :model "YT-1300f light freighter"}}
#:person{:first "Han" :last "Solo" :ship #:ship{:name "Millenium Falcon" :model "YT-1300f light freighter"}}

;; effects on keywords with ns, without ns, with _ ns, and non-kw
user=> #:foo{:kw 1, :n/kw 2, :_/bare 3, 0 4}
{:foo/kw 1, :n/kw 2, :bare 3, 0 4}

;; auto-resolved namespaces (will use user as the ns)
user=> #::{:kw 1, :n/kw 2, :_/bare 3, 0 4}
:user/kw 1, :n/kw 2, :bare 3, 0 4}

;; auto-resolve alias s to clojure.string
user=> (require '[clojure.string :as s])
nil
user=> #::s{:kw 1, :n/kw 2, :_/bare 3, 0 4}
{:clojure.string/kw 1, :n/kw 2, :bare 3, 0 4}

;; to show symbol changes, we'll quote the whole thing to avoid evaluation
user=> '#::{a 1, n/b 2, _/c 3}
{user/a 1, n/b 2, c 3}

;; edn reader also supports (only) the #: syntax
user=> (clojure.edn/read-string "#:person{:first \"Han\" :last \"Solo\" :ship #:ship{:name \"Millenium Falcon\" :model \"YT-1300f light freighter\"}}")
#:person{:first "Han", :last "Solo", :ship #:ship{:name "Millenium Falcon", :model "YT-1300f light freighter"}}

Patch: clj-1910-2.patch

Screener notes:

  • Autoresolution supports fully-qualified loaded namespaces (like auto-resolved keywords)
  • TODO: pprint support for namespaced maps
  • TODO: printer flag to suppress printing namespaced maps


 Comments   
Comment by Nicola Mometto [ 08/Apr/16 3:57 AM ]

1- yes please. that's consistent with how tagged literals work.
2- no please. that would make the proposed syntax useless for e.g. Datomic schemas, for which I think this would be a good fit to reduce noise
3- yes please
4- yes please, consistency over print methods is important

Comment by Nicola Mometto [ 08/Apr/16 4:00 AM ]

Quoting from a post I wrote on the clojure-dev ML:

  • I really don't like the idea of special-casing `_` here, users are already confused about idioms like `[.. & _]` thinking that `_` is some special token/variable. Making it an actual special token in some occasion wouldn't help.
  • I also don't like how we're making the single `:` auto-qualify keywords when used within the context of a qualifying-map. Auto-qualifying keywords has always been the job of the double `::`, changing this would introduce (IMO) needless cognitive overhead.
  • The current impl treats `#:foo{'bar 1}` and `'#:foo{bar 1}` differently. I can see why is that, but the difference might be highly unintuitive to some.
  • The current proposal makes it feel like quote is auto-qualifying symbols , when that has always been the job of syntax-quote. I know that's not correct, but that's how it's perceived.

Here's an alternative syntax proposal that handles all those issues:

  • No #::, only #:foo or #::foo
  • No auto-resolution of symbols when the namespaced-map is quoted, only when syntax-quoted
  • No special-casing of `_`
  • No auto-resolution of single-colon keywords

Here's how the examples in the ticket description would look:

#:person{::first "Han", ::last "Solo", ::ship #:ship{::name "Millenium Falcon", ::model "YT-1300f light freighter"}}
;=> {:person/first "Han" :person/last "Solo" :person/ship {:ship/name "Millenium Falcon" :ship/model "YT-1300f light freighter"}}

#:foo{::kw 1, :n/kw 2, :bare 3, 0 4}
;=> {:foo/kw 1, :n/kw 2, :bare 3, 0 4}

{::kw 1, :n/kw 2, bare 3, 0 4}
;=> {:user/kw 1, :n/kw 2, :bare 3, 0 4}

Note in the previous example how we don't need `#::` at all – `::` already does that job for us

(require '[clojure.string :as s])
#::s{::kw 1, :n/kw 2, bare 3, 0 4}
;=> {:clojure.string/kw 1, :n/kw 2, :bare 3, 0 4}

`{a 1, n/b 2, ~'c 3}
;=> {user/a 1, n/b 2, c 3}

Again, no need for `#::` here, we can just rely on the existing auto-qualifying behaviour of `.

`#:foo{a 1, n/b 2}
;=> {foo/a 1, n/b 2}

I think this would be more consistent with the existing behaviour – it's basically just making `#:foo` or `#::foo` mean: in the top-level keys of the following map expression, resolve keywords/symbols as if ns was bound to `foo`, rather than introducing new resolution rules and special tokens.

I realize that this proposal wouldn't work with EDNReader as-is, given its lack of support for `::` and "`". I don't have a solution to that other than "let's just bite the bullet and implement them there too", but maybe that's not acceptable.

Comment by Alex Miller [ 08/Apr/16 8:45 AM ]

Nicola, thanks for the proposal, we talked through it. We share your dislike for :_/kw syntax and you should consider that a placeholder for this behavior for the moment - it may be removed or replaced before we get to a published release.

For the rest of it:

  • requiring syntax quote is a non-starter
  • supporting a mixture of default ns and the current ns is important and this is not possible with your proposal. Like #:foo{:bar 1 ::baz 2}.
  • there is a lot of value to changing the scope of a map without modifying the contents, which is an advantage of the syntax in the ticket
Comment by Christophe Grand [ 08/Apr/16 10:31 AM ]

Why restrict this feature to a single namespace? (this doesn't preclude a shorthand for the single mapping) I'd like to locally define aliases (and default ns).

Comment by Alex Miller [ 08/Apr/16 11:02 AM ]

We already have namespace level aliases. You can use :: in the map to leverage those aliases (independently from the default ns):

(ns app 
  (:require [my.domain :as d]
            [your.domain :as y]))

#::{:svc 1, ::d/name 2, ::y/name 3}

;;=> {:app/svc 1, :my.domain/name 2, :your.domain/y 3}
Comment by Christophe Grand [ 11/Apr/16 4:03 AM ]

Alex, if existing namespace level aliases are enough when there's more than one namespace used in the key set I fail to understand the real value of this proposal.

Okay I'm lying a little: there are no aliases in edn, so this would bring aliases to edn (and allows printers to factor/alias namespaces out). And for Clojure code you can't define an alias to a non-existing namespace – and I believe that this implementation wouldn't check namespace existence when resolving the default ns #:person{:name}.

Still my points hold for edn (and that's where the value of this proposal seems to be): why not allows local aliases too?

#:person #:employee/e {:name "John Smith", :e/eid "012345"}
;=> {:person/name "John Smith", :employee/eid "012345"}

I have another couple of questions:

  • should it apply to other datatypes?
  • should it be transitive?
Comment by Alex Miller [ 28/Apr/16 1:33 PM ]

New patch rev supports spaces between the namespace part #:foo and the map in both LispReader and EdnReader.





[CLJ-1243] Cannot resolve public generic method from package-private base class Created: 01/Aug/13  Updated: 18/Jun/16

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

Type: Defect Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: interop

Attachments: GZip Archive clj-1243-demo1.tar.gz    

 Description   

The Clojure compiler cannot resolve a public generic method inherited from a package-private base class.

Instructions to reproduce:

  • In package P1
    • Define a package-private class A with generic type parameters
    • Define a public method M in A using generic types in either its arguments or return value
    • Define a public class B which extends A
  • In package P2
    • Construct an instance of B
    • Invoke B.M()

This is valid in Java. In Clojure, invoking B.M produces a reflection warning, followed by the error "java.lang.IllegalArgumentException: Can't call public method of non-public class." No amount of type-hinting prevents the warning or the error.

Attachment clj-1243-demo1.tar.gz contains sample code and script to demonstrate the problem.

Examples of Java projects which use public methods in package-private classes:



 Comments   
Comment by Stuart Sierra [ 01/Aug/13 5:11 PM ]

It is also not possible to call the method reflectively from Java.

This may be a bug in Java reflection: JDK-4283544

But why does it only happen on generic methods?

Comment by Stuart Sierra [ 08/Aug/13 11:59 AM ]

According to Rich Hickey, the presence of bridge methods is unspecified and inconsistent across JDK versions.

A possible solution is to use ASM to examine the bytecode of third-party Java classes, instead of the reflection API. That way the Clojure compiler would have access to the same information as the Java compiler.

Comment by Andy Fingerhut [ 17/Nov/13 11:01 PM ]

CLJ-1183 was closed as a duplicate of this one. Mentioning it here in case anyone working on this ticket wants to follow the link to it and read discussion or test cases described there.

Comment by Noam Ben Ari [ 21/Feb/15 4:55 AM ]

The current work around I use is to define a new Java class, add a static method that does what I need, and call that from Clojure.

Comment by Noam Ben Ari [ 21/Feb/15 9:28 AM ]

Also, I'm seeing this issue in 1.6 and 1.7(alpha5) but the issue mentions only up to 1.5 .

Comment by Adam Tait [ 03/Apr/16 5:32 PM ]

Just ran into this issue trying to use Google's Cloud APIs.
To use Google's Cloud Datastore, you need to access the .kind method on a protected generic subclass (BaseKey), to which KeyFactory extends.

Tested on both Clojure 1.7 & 1.8 at runtime, the following exception persists;

IllegalArgumentException Can't call public method of non-public class: public com.google.gcloud.datastore.BaseKey$Builder com.google.gcloud.datastore.BaseKey$Builder.kind(java.lang.String) clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:88)

Comment by Kai Strempel [ 18/Jun/16 1:19 PM ]

I ran into the exact same issue with Google's Cloud API's.

Tested it with 1.8 and with 1.9.0-alpha7. Same Problem.

Comment by Kai Strempel [ 18/Jun/16 1:19 PM ]

I ran into the exact same issue with Google's Cloud API's.

Tested it with 1.8 and with 1.9.0-alpha7. Same Problem.





[CLJ-1312] clojure.string/split on empty string includes empty string in results Created: 21/Dec/13  Updated: 17/Jun/16  Resolved: 21/Dec/13

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

Type: Defect Priority: Minor
Reporter: Russell Dunphy Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: string


 Description   

Splitting a string using clojure.string/split with an empty regex includes the empty string in the results - is this expected behaviour?

Example:

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
user=> (clojure.string/split "abc" #"")
["" "a" "b" "c"]


 Comments   
Comment by Alex Miller [ 21/Dec/13 8:05 AM ]

Yes, I think so. This is a case where Clojure defers to the host (Java) for behavior. I think the way to interpret this is that the empty pattern matches all strings. Split checks left to right whether there is a next chunk of string that matches the pattern. The empty pattern matches at the beginning to a string of length 0. Something like that.

Comment by Mark Engelberg [ 07/Sep/14 12:27 PM ]

This bug is a real problem, because it works differently on Windows than on Linux. On Windows, clojure.string/split behaves exactly as you'd expect:

user=> (clojure.string/split "abc" #"")
["a" "b" "c"]

Only on Linux do you get the strange behavior where the empty string shows up at the beginning of the list.

I recently had a student that got burned by this in some webserver code that relied on splitting using the empty regex. It performed flawlessly on her local Windows machine, but mysteriously broke when she uploaded the uberwar to the cloud. The bug was very difficult to track down.

If this were a bug on both Windows and Linux, at least you could plan around it. But right now, it's an obstacle to Clojure's capability of running consistently across platforms.

Comment by Mark Engelberg [ 07/Sep/14 12:40 PM ]

Upon further research, I've found that this is not a Windows/Linux issue, rather it's a difference between Java 7 and Java 8. On Java 8, splitting with the empty string no longer produces a sequence that begins with an empty string.

As you said before, this is just a gotcha relating to Java, not a Clojure issue.

Comment by Ahmed Fasih [ 17/Jun/16 1:08 PM ]

This happens in ClojureScript 1.8.51:

(clojure.string/split "qwe" #"") ; => ["" "q" "w" "e"]





[CLJ-1962] fn-spec only works with a fully ns-qualified quoted symbol Created: 15/Jun/16  Updated: 16/Jun/16  Resolved: 16/Jun/16

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

Type: Defect Priority: Major
Reporter: Laszlo Török Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: spec

Approval: Vetted

 Description   

fn-spec no longer does symbol resolution on its parameter.

However, the following

(sp/fdef + :args (sp/cat :operand (sp/* number?)))

(sp/fn-spec +) ;; => nil (A)
(sp/fn-spec '+) ;; => nil (B)
(sp/fn-spec 'clojure.core/+) ;; this actually returns the fn-specs

Proposal: Either resolve the symbol/var or document that fully-qualified is required.

Also see:
https://gist.github.com/laczoka/acd65028f5a46338e33c940d49d01753



 Comments   
Comment by Laszlo Török [ 15/Jun/16 11:32 AM ]

thanks Alex for making the ticket more palatable

Comment by Alex Miller [ 16/Jun/16 8:39 AM ]

fn-spec has been renamed to get-spec in master and works a bit differently than before. However, it requires a qualified symbol, keyword, or var.

If you want resolution in terms of the local namespace when invoking it, use ` as a helper:

(sp/get-spec `plus)
Comment by Laszlo Török [ 16/Jun/16 4:13 PM ]

Fantastic!





[CLJ-1960] Bug in clojure.core/mod with large Double argument Created: 14/Jun/16  Updated: 15/Jun/16

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

Type: Defect Priority: Minor
Reporter: William Tozier Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math, numerics
Environment:

Java 8 update 91 on Mac OS X 10.11.5


Approval: Triaged

 Description   

The `clojure.core/mod` function works just as expected for small positive floating-point dividend and small positive integer divisor. But today I was working on some edge case tests and came across the following inexplicable behavior:

REPL_session
user=> (def big  Double/MAX_VALUE)
#'user/big
user=> (mod big 10)
0.0
user=> (mod big 100)
0.0
user=> (mod big 1000)
1.9958403095347198E292
user=> (mod big 999)
-Infinity
user=> (mod big 998)
0.0
user=> (mod big 997)
1.9958403095347198E292
user=> (mod big 996)
0.0
user=> (mod big 995)
0.0
user=> (mod big 994)
0.0
user=> (mod big 1001)
1.9958403095347198E292
user=> (mod big 1002)
0.0
user=> (mod big 1003)
0.0
user=> (mod big 1004)
-Infinity
user=> (mod big 1005)
0.0

No idea whether this is inherited from a Java bug. I can see nothing special about the values chosen, and I suspect if one scanned it'd be easy to find other glitches.



 Comments   
Comment by