<< Back to previous view

[CLJ-2042] s/form of s/? does not resolve pred Created: 14/Oct/16  Updated: 28/Oct/16  Resolved: 28/Oct/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: 0
Labels: spec

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

 Description   
user=> (require '[clojure.spec :as s])
nil
user=> (s/form (s/? int?))
(clojure.spec/? int?)

Patch: clj-2042.patch






[CLJ-2032] Confusing error conforming fspec with missing arg spec Created: 03/Oct/16  Updated: 28/Oct/16  Resolved: 28/Oct/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: 0
Labels: errormsgs, spec
Environment:

1.9.0-alpha13


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

 Description   
(require '[clojure.spec :as s])
(def my-spec (s/fspec :ret string?))
(s/conform my-spec (fn [j] (str j)))
IllegalArgumentException No implementation of method: :specize* of protocol: #'clojure.spec/Specize found for class: nil  clojure.core/-cache-protocol-fn (core_deftype.clj:568)
	clojure.core/-cache-protocol-fn (core_deftype.clj:583)
	clojure.spec/fn--13560/G--13555--13569 (spec.clj:121)
	clojure.spec/specize (spec.clj:138)
	clojure.spec/gensub (spec.clj:262)
	clojure.spec/gen (spec.clj:275)
	clojure.spec/gen (spec.clj:275)
	clojure.spec/validate-fn (spec.clj:1664)
	clojure.spec/fspec-impl/reify--14270 (spec.clj:1686)
	clojure.spec/conform (spec.clj:150)
	clojure.spec/conform (spec.clj:146)

Proposed: When conforming, throw if no args spec specified for the fspec:

Can't conform fspec without args spec: (fspec :args nil :ret string? :fn nil)

Alternatives

  • absence of args spec always conforms ::invalid
  • absence of args spec always conforms any args
  • disallow fspecs without args (still potentially useful for other uses like documentation, so not sure we want to do that)

Patch: clj-2032.patch






[CLJ-2027] bean printing regression from namespace map printing Created: 24/Sep/16  Updated: 28/Oct/16  Resolved: 28/Oct/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: Trey Sullivan Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: print, regression
Environment:

Clojure 1.9.0-alpha12


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

 Description   

The new namespace map printing is causing a failure in printing bean maps (which are proxies that don't support empty):

user=> (bean (java.util.Date.))
UnsupportedOperationException empty  clojure.core.proxy$clojure.lang.APersistentMap$ff19274a.empty (:-1)

user=> (pst *e)
UnsupportedOperationException empty
	clojure.core.proxy$clojure.lang.APersistentMap$ff19274a.empty (:-1)
	clojure.core/empty (core.clj:5151)
	clojure.core/lift-ns (core_print.clj:237)

Cause: The internal lift-ns function calls empty on the map too early (here it doesn't need to call it at all).

Proposed: Defer calling (empty m) until we know map has namespace keywords and namespace maps will be used for printing.

Patch: clj-2027.patch (note that into is not used in the change b/c it has not yet been defined at this point)






[CLJ-2024] Check should specize function specs before checking Created: 19/Sep/16  Updated: 28/Oct/16  Resolved: 28/Oct/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: James Reeves Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: spec

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

 Description   

This code works fine in 1.9.0-alpha12:

(defn f [x] (+ x 1))
(s/def f (s/fspec :args (s/cat :x number?) :ret number?))
(stest/check `f)

But if we factor the fspec out into its own keyword:

(defn f [x] (+ x 1))
(s/def ::inc (s/fspec :args (s/cat :x number?) :ret number?))
(s/def f ::inc)
(stest/check `f)

The check fails with the exception:

({:failure #error {
 :cause "No :args spec"
 :data #:clojure.spec{:failure :no-args-spec}
 :via
 [{:type clojure.lang.ExceptionInfo
   :message "No :args spec"
   :data #:clojure.spec{:failure :no-args-spec}
   :at [...]}]
 :trace
 [...]}, :sym user/f, :spec :user/inc})

The check function doesn't seem to be resolving ::inc, when presumably it should.

Patch: clj-2024-2.patch



 Comments   
Comment by Rich Hickey [ 28/Oct/16 7:44 AM ]

this should be fixed in fspec, not its use by test

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

fspec is not the problem as far as I can tell - it is already making specs of its args.

The problem is that f is registered as an alias of ::inc. I don't think you want to resolve that at registration time (as ::inc might not exist yet).

The problem as far as I understand it is that at the time of use (by check), f is not resolved to it's final spec and that's what the patch does.

Comment by Alex Miller [ 28/Oct/16 8:43 AM ]

Added new patch that uses `spec` instead of private `specize` function.





[CLJ-2012] ns spec breaks gen-class forms that use strings as class names Created: 24/Aug/16  Updated: 07/Sep/16  Resolved: 07/Sep/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: Nicola Mometto Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: regression, spec

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

 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.

Approach: Pull out spec for class identifiers which can be either a simple-symbol (class name) or a string and use that in signature (which is used for both gen-class constructors and methods.

Patch: clj-2012-2.patch



 Comments   
Comment by Stuart Halloway [ 26/Aug/16 7:23 AM ]

Nicola: Good catch, thanks for the report!

Alex: argtype is a pretty general name to grab at the top level spec ns. How about something like class-ident?





[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: 07/Sep/16  Resolved: 07/Sep/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: lvh Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: spec
Environment:

N/A


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

 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.

Comment by Stuart Halloway [ 26/Aug/16 7:31 AM ]

lvh: thanks for the report!

Not sure that we want to give up on generatively testing macros, but it certainly doesn't work atm, so this seems good until we take it on.





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

Status: Closed
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: Completed Votes: 0
Labels: docstring, spec
Environment:

N/A


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

 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.

Comment by Stuart Halloway [ 26/Aug/16 7:34 AM ]

Thanks again lvh!





[CLJ-2004] s/multi-spec doesn't include :retag in `s/form` Created: 18/Aug/16  Updated: 07/Sep/16  Resolved: 07/Sep/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: Allen Rohner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: spec

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

 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.

(require '[clojure.spec :as s])
(defmulti mm :mm/type)
(s/def ::foo (s/multi-spec mm :mm/type))
(s/form ::foo)
;; (clojure.spec/multi-spec user/mm) ;; :mm/type is missing

Approach: Include retag in form

Patch: clj-2004.patch



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

Yeah, this is one of many known form issues.





[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-1988] collection specs conform to reversed list when used on a sequence Created: 24/Jul/16  Updated: 07/Sep/16  Resolved: 07/Sep/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: Greg Chapman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: spec

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

 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-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-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-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-1967] Enhanced namespaced map pprint support Created: 23/Jun/16  Updated: 31/Aug/16  Resolved: 31/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: Unassigned
Resolution: Completed Votes: 0
Labels: print

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

 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-1958] Add uri? generator Created: 12/Jun/16  Updated: 14/Jun/16  Resolved: 14/Jun/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: Unassigned
Resolution: Completed Votes: 0
Labels: generator, spec

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

 Description   

uri? was added as a predicate in 1.9 but doesn't have a mapped spec generator.

Proposed: Generate uuids, then produce URIs of the form "http://<uuid>.com".

Patch: clj-1958.patch



 Comments   
Comment by Alex Miller [ 14/Jun/16 11:02 AM ]

Applied for 1.9.0-alpha6.





[CLJ-1957] Add gen support for bytes? Created: 11/Jun/16  Updated: 14/Jun/16  Resolved: 14/Jun/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: Unassigned
Resolution: Completed Votes: 0
Labels: generator, spec

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

 Description   

The generator for the new bytes? predicate was overlooked.



 Comments   
Comment by Alex Miller [ 14/Jun/16 11:02 AM ]

Applied for 1.9.0-alpha6.





[CLJ-1937] spec/fn-specs should behave the same as s/spec w.r.t not-found Created: 28/May/16  Updated: 14/Jun/16  Resolved: 14/Jun/16

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

Type: Enhancement Priority: Minor
Reporter: Allen Rohner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: spec

Approval: Vetted

 Description   

s/spec and s/fn-specs behave differently for 'not-found' values:

(s/spec ::bogus)
=> Exception Unable to resolve spec: :user/bogus  clojure.spec/the-spec (spec.clj:95)

(s/fn-specs 'bogus)
=> {:args nil, :ret nil, :fn nil}

fn-specs should throw or return nil

Note: doc uses the return of fn-specs so needs to be checked that it still works properly if this changes



 Comments   
Comment by Alex Miller [ 13/Jun/16 3:44 PM ]

There will be some updates to fn-specs soon and this should be included.

Comment by Alex Miller [ 14/Jun/16 9:20 AM ]

As of https://github.com/clojure/clojure/commit/92df7b2a72dad83a901f86c1a9ec8fbc5dc1d1c7, fn-spec (was fn-specs) now returns nil if no fn spec is found.





[CLJ-1935] clojure.spec/multi-spec ignores the multimethod hierarchy Created: 28/May/16  Updated: 07/Sep/16  Resolved: 07/Sep/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: Viktor Magyari Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: multimethods, spec

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

 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.

Cause: The implementation of multi-spec uses a predicate that invokes the dispatch function, then looks up the result in the method table. However this does not leverage the actual logic used in multimethods for hierarchy resolution.

Approach: Replace the lookup in the method table with a call to getMethod(), which will use the same lookup logic that multimethod uses.

Patch: clj-1935.patch



 Comments   
Comment by Rich Hickey [ 06/Sep/16 12:20 PM ]

dval should change similarly

Comment by Alex Miller [ 06/Sep/16 1:41 PM ]

dval was and is correct, I think? it's just the means of looking up a match for the dispatch value.





[CLJ-1934] (s/cat) with nonconforming data causes infinite loop in explain-data Created: 27/May/16  Updated: 01/Jun/16  Resolved: 01/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: Sven Richter Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: spec
Environment:

Ubuntu 15.10
Leiningen 2.6.1 on Java 1.8.0_91 Java HotSpot(TM) 64-Bit Server VM


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

 Description   

The following code yields an infinite loop

(require '[clojure.spec :as s])
(s/explain-data (s/cat) [1]) ;; infinite loop
​

Thread dump:

"main" prio=5 tid=0x00007fb602000800 nid=0x1703 runnable [0x0000000102b3f000]
   java.lang.Thread.State: RUNNABLE
	at clojure.lang.RT.seqFrom(RT.java:529)
	at clojure.lang.RT.seq(RT.java:524)
	at clojure.core$seq__5444.invokeStatic(core.clj:137)
	at clojure.core$concat$cat__5535$fn__5536.invoke(core.clj:715)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	- locked <0x000000015885e4e0> (a clojure.lang.LazySeq)
	at clojure.lang.LazySeq.seq(LazySeq.java:56)
	- locked <0x000000015885e2f0> (a clojure.lang.LazySeq)
	at clojure.lang.RT.seq(RT.java:522)
	at clojure.core$seq__5444.invokeStatic(core.clj:137)
	at clojure.core$map$fn__5872.invoke(core.clj:2637)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	- locked <0x000000015885e3b0> (a clojure.lang.LazySeq)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	- locked <0x000000015885e3b0> (a clojure.lang.LazySeq)
	at clojure.lang.ChunkedCons.chunkedNext(ChunkedCons.java:59)
	at clojure.lang.ChunkedCons.next(ChunkedCons.java:43)
	at clojure.lang.RT.next(RT.java:689)
	at clojure.core$next__5428.invokeStatic(core.clj:64)
	at clojure.core$dorun.invokeStatic(core.clj:3033)
	at clojure.core$doall.invokeStatic(core.clj:3039)
	at clojure.walk$walk.invokeStatic(walk.clj:46)
	at clojure.walk$postwalk.invokeStatic(walk.clj:52)
	at clojure.spec$abbrev.invokeStatic(spec.clj:114)
	at clojure.spec$re_explain.invokeStatic(spec.clj:1286)
	at clojure.spec$regex_spec_impl$reify__11725.explain_STAR_(spec.clj:1305)
	at clojure.spec$explain_data_STAR_.invokeStatic(spec.clj:143)
	at clojure.spec$spec_checking_fn$conform_BANG___11409.invoke(spec.clj:528)
	at clojure.spec$spec_checking_fn$fn__11414.doInvoke(spec.clj:540)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at user$eval8.invokeStatic(NO_SOURCE_FILE:5)
	at user$eval8.invoke(NO_SOURCE_FILE:5)
	at clojure.lang.Compiler.eval(Compiler.java:6942)
	at clojure.lang.Compiler.eval(Compiler.java:6905)
	at clojure.core$eval.invokeStatic(core.clj:3105)
	at clojure.core$eval.invoke(core.clj:3101)
	at clojure.main$repl$read_eval_print__8495$fn__8498.invoke(main.clj:240)
	at clojure.main$repl$read_eval_print__8495.invoke(main.clj:240)
	at clojure.main$repl$fn__8504.invoke(main.clj:258)
	at clojure.main$repl.invokeStatic(main.clj:258)
	at clojure.main$repl_opt.invokeStatic(main.clj:322)
	at clojure.main$main.invokeStatic(main.clj:421)
	at clojure.main$main.doInvoke(main.clj:384)
	at clojure.lang.RestFn.invoke(RestFn.java:397)
	at clojure.lang.Var.invoke(Var.java:375)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)

Cause: This line in op-describe:

(cons `cat (mapcat vector (c/or (seq ks) (repeat :_)) (c/or (seq forms) (repeat nil)))))

is called here with no ks or form, so will mapcat vector over infinite streams of :_ and nil.

Approach: check for this case and avoid doing that

Patch: clj-1934.patch



 Comments   
Comment by Alex Miller [ 01/Jun/16 6:26 AM ]

This was fixed via an alternate change in 1.9.0-alpha4.





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

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

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

Approval: Vetted

 Description   

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



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

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

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

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





[CLJ-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-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-1910] Namespaced maps Created: 07/Apr/16  Updated: 23/Sep/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.

Comment by Tim Gilbert [ 23/Sep/16 1:12 AM ]

Any news on the flag to prevent this behavior (mentioned in the screener notes)?

I ask because (pr-str) in Clojure 1.9.0-alpha12 currently produces EDN that can't be consumed by (cljs.reader/read-string) in ClojureScript 1.9.229 (the ClojureScript consumption side is tracked in CLJS-1706).

It would be nice to have a way to disable this behavior until ClojureScript reaches parity, and a lot of fairly widely-used open-source libraries use plain (pr-str) to generate EDN output - for example, ring-middleware-format.

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

Yes, that was implemented in a separate ticket (CLJ-1993) in Clojure 1.9.0-alpha11. There is now a print-namespace-maps dynvar. It defaults to false, but is set to true in the standard REPL.

So at the REPL you can (set! print-namespace-maps false) to turn it off.

If you're doing stuff in your program outside a REPL, it will be off by default so you probably don't need to do anything as of alpha11.

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

that's *print-namespace-maps* above, sorry for the formatting





[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-1790] Error extending protocols to Java arrays Created: 29/Jul/15  Updated: 28/Oct/16  Resolved: 28/Oct/16

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

Type: Defect Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: compiler, protocols

Attachments: Text File 0001-CLJ-1790-emit-a-cast-to-the-interface-during-procol-.patch    
Patch: Code
Approval: Ok

 Description   

First reported from core.matrix, but here's a smaller repro:

user=> (defprotocol p (f [_]))
p
user=> (fn [] (f (object-array [])))

VerifyError (class: user$eval15920$fn__15921, method: invoke signature: ()Ljava/lang/Object;) Incompatible object argument for function call  user/eval15920 (form-init9183379085801704163.clj:1)

Cause: The jvm verifier doesn't like situations where we have an array on the stack typed as such, and on a later codepath it is used as target for an invokeinterface even if that path is unreachable because of a previous instance check.

Here's an explanation of exactly our case in pseudo bytecode:

load obj // Object[]
 dup
 instanceof SomeInterface
 iftruejmp label1
 pop
 jmp end
label1:
 // here is where the verifier chokes.
 // it can figure out that the target is of type Object[] which can never be a SomeInterface
 // but it cannot figure out that this code path can never be reached because of the previous
 // instance check with jump
 // to fix this we need to insert an explicit checkcast to SomeInterface on the target
 invokeinterface SomeInterface/someMethod
end:
 return

Proposed: Insert an explicit checkcast to the interface on the target.

Also see: CLJ-1381

Patch: 0001-CLJ-1790emit-a-cast-to-the-interface-during-procol.patch



 Comments   
Comment by Nicola Mometto [ 06/Nov/15 3:53 PM ]

Mike Anderson does 1.8.0-beta2 fix this issue?
Alex Miller if core.matrix is still affected this must be fixed before 1.8.0 as it'd mean that direct linking is still broken

Comment by Nicola Mometto [ 06/Nov/15 6:26 PM ]

I could reproduce the bug with 1.8.0-beta2 btw

Comment by Nicola Mometto [ 06/Nov/15 7:27 PM ]

Apparently this is not a 1.8 regression.

At least 1.6 and 1.7 both manifest the same issue:

Clojure 1.6.0
user=> (defprotocol p (f [_]))
p
user=> (fn [] (f (object-array [])))

VerifyError (class: user$eval15920$fn__15921, method: invoke signature: ()Ljava/lang/Object;) Incompatible object argument for function call  user/eval15920 (form-init9183379085801704163.clj:1)

Comment by Michael Blume [ 06/Nov/15 8:24 PM ]

Do we know why core.matrix works with Clojure 1.6/1.7 then?

Comment by Nicola Mometto [ 06/Nov/15 9:09 PM ]

It doesn't.

Clojure 1.7.0
Java HotSpot(TM) 64-Bit Server VM 1.8.0_60-b27

user=> (require 'clojure.core.matrix.protocols)
nil
user=> (clojure.core.matrix.protocols/construct-matrix (object-array 1) [1])

VerifyError (class: user$eval6935, method: invoke signature: ()Ljava/lang/Object;) Incompatible object argument for function call  java.lang.Class.getDeclaredConstructors0 (Class.java:-2)
user=>

I attached a patch that fixes this issue.
It's caused by the jvm verifier understanding that the object on the stack is an array and thus can never be an instance of the protcol interface, but not being able to understant that the code path leading to the direct protocol interface method invocation can never be reached because of a branch guided by an instance check for that interface on the target

Comment by Mike Anderson [ 06/Nov/15 10:10 PM ]

Apologies, it is possible I just hadn't tested this code path thoroughly before.

It only seems to get triggered in certain circumstances, the following behaviour is interesting:

=> (let [o (identity (object-array 1))]
     (clojure.core.matrix.protocols/dimensionality o))
1
=> (let [o (object-array 1)]
     (clojure.core.matrix.protocols/dimensionality o))
VerifyError (class: clojure/core/matrix$eval17775, method: invokeStatic signature: ()Ljava/lang/Object;) Incompatible object argument for function call  java.lang.Class.getDeclaredConstructors0 (:-2)

Perhaps it only happens when the callsite has type information about the protocol parameter?

Comment by Nicola Mometto [ 07/Nov/15 4:53 AM ]

Correct, apparently the jvm verifier doesn't like situations where we have an array on the stack typed as such, and on a later codepath it is used as target for an invokeinterface even if that path is unreachable because of a previous instance check.

here's an explaination of exactly our case in pseudo bytecode:

..
 load obj // Object[]
 dup
 instanceof SomeInterface
 iftruejmp label1
 pop
 jmp end
label1:
 // here is where the verifier chokes.
 // it can figure out that the target is of type Object[] which can never be a SomeInterface
 // but it cannot figure out that this code path can never be reached because of the previous
 // instance check with jump
 // to fix this we need to insert an explicit checkcast to SomeInterface on the target
 invokeinterface SomeInterface/someMethod
end:
 return
Comment by Stuart Halloway [ 14/Oct/16 3:04 PM ]

Thanks for the tight repro!

I am marking this screened because the explanation makes sense and the patch works as advertised, but will caveat that I don't have a deep understanding here.





[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-1719] Add clojure.core/boolean? predicate Created: 03/May/15  Updated: 07/Jun/16  Resolved: 07/Jun/16

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

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

 Description   

Having this predicate aids with clj/cljs compatibility.



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

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

Comment by Alex Miller [ 07/Jun/16 11:39 AM ]

Added in https://github.com/clojure/clojure/commit/58227c5de080110cb2ce5bc9f987d995a911b13e for 1.9.0-alpha5.





[CLJ-1673] Improve clojure.repl/dir-fn to work on namespace aliases in addition to canonical namespaces. Created: 11/Mar/15  Updated: 07/Sep/16  Resolved: 07/Sep/16

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

Type: Enhancement Priority: Minor
Reporter: Jason Whitlark Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: ft, repl

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

 Description   

Extend clojure.repl/dir to work with the aliases in the current namespace

After:

user=> (require '[clojure.string :as s])
nil
user=> (dir s)
blank?
capitalize
...etc

Patch: clj-1673-3.patch

Screened by: Alex Miller



 Comments   
Comment by Jason Whitlark [ 11/Mar/15 4:00 PM ]

Possible unit test, since clojure.string is aliased in the test file:

(is (= (dir-fn 'clojure.string) (dir-fn 'str)))

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

Please add a test...

Comment by Jason Whitlark [ 02/May/15 10:35 PM ]

Updated patch, tweaked dir-fn, added test.

Comment by Jason Whitlark [ 02/May/15 10:38 PM ]

Should be good to go. I removed the old patch.

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

Tweaked patch just to remove errant blank line, otherwise same.

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

Applied this patch directly to master, test failure:

[java] ERROR in (test-dir) (core.clj:4023)
     [java] expected: (= (dir-fn (quote clojure.string)) (dir-fn (quote str)))
     [java]   actual: java.lang.Exception: No namespace: str found
     [java]  at clojure.core$the_ns.invokeStatic (core.clj:4023)
     [java]     clojure.repl$dir_fn.invokeStatic (repl.clj:186)
     [java]     clojure.repl$dir_fn.invoke (repl.clj:-1)
     [java]     clojure.test_clojure.repl$fn__21967$fn__21976.invoke (repl.clj:28)
     [java]     clojure.core$with_redefs_fn.invokeStatic (core.clj:7211)
     [java]     clojure.core$with_redefs_fn.invoke (core.clj:-1)
     [java]     clojure.test_clojure.repl$fn__21967.invokeStatic (repl.clj:27)
     [java]     clojure.test_clojure.repl/fn (repl.clj:-1)
     [java]     clojure.test$test_var$fn__7962.invoke (test.clj:703)
     [java]     clojure.test$test_var.invokeStatic (test.clj:703)
     [java]     clojure.test$test_var.invoke (test.clj:-1)
     [java]     clojure.test$test_vars$fn__7984$fn__7989.invoke (test.clj:721)
     [java]     clojure.test$default_fixture.invokeStatic (test.clj:673)
     [java]     clojure.test$default_fixture.invoke (test.clj:-1)
     [java]     clojure.test$test_vars$fn__7984.invoke (test.clj:721)
     [java]     clojure.test$default_fixture.invokeStatic (test.clj:673)
     [java]     clojure.test$default_fixture.invoke (test.clj:-1)
     [java]     clojure.test$test_vars.invokeStatic (test.clj:717)
     [java]     clojure.test$test_all_vars.invokeStatic (test.clj:723)
     [java]     clojure.test$test_ns.invokeStatic (test.clj:744)
     [java]     clojure.test$test_ns.invoke (test.clj:-1)
     [java]     clojure.core$map$fn__4770.invoke (core.clj:2639)
     [java]     clojure.lang.LazySeq.sval (LazySeq.java:40)
     [java]     clojure.lang.LazySeq.seq (LazySeq.java:49)
     [java]     clojure.lang.Cons.next (Cons.java:39)
     [java]     clojure.lang.RT.next (RT.java:688)
     [java]     clojure.core$next__4327.invokeStatic (core.clj:64)
     [java]     clojure.core$reduce1.invokeStatic (core.clj:924)
     [java]     clojure.core$reduce1.invokeStatic (core.clj:914)
     [java]     clojure.core$merge_with.invokeStatic (core.clj:2943)
     [java]     clojure.core$merge_with.doInvoke (core.clj:-1)
     [java]     clojure.lang.RestFn.applyTo (RestFn.java:139)
     [java]     clojure.core$apply.invokeStatic (core.clj:647)
     [java]     clojure.test$run_tests.invokeStatic (test.clj:754)
     [java]     clojure.test$run_tests.doInvoke (test.clj:-1)
     [java]     clojure.lang.RestFn.applyTo (RestFn.java:137)
     [java]     clojure.core$apply.invokeStatic (core.clj:645)
     [java]     clojure.core$apply.invoke (core.clj:-1)
     [java]     user$eval29133.invokeStatic (run_test.clj:8)
     [java]     user$eval29133.invoke (run_test.clj:-1)
     [java]     clojure.lang.Compiler.eval (Compiler.java:6934)
     [java]     clojure.lang.Compiler.load (Compiler.java:7381)
     [java]     clojure.lang.Compiler.loadFile (Compiler.java:7319)
     [java]     clojure.main$load_script.invokeStatic (main.clj:275)
     [java]     clojure.main$script_opt.invokeStatic (main.clj:335)
     [java]     clojure.main$script_opt.invoke (main.clj:-1)
     [java]     clojure.main$main.invokeStatic (main.clj:421)
     [java]     clojure.main$main.doInvoke (main.clj:-1)
     [java]     clojure.lang.RestFn.invoke (RestFn.java:408)
     [java]     clojure.lang.Var.invoke (Var.java:379)
     [java]     clojure.lang.AFn.applyToHelper (AFn.java:154)
     [java]     clojure.lang.Var.applyTo (Var.java:700)
     [java]     clojure.main.main (main.java:37)
Comment by Jason Whitlark [ 12/Oct/15 7:22 PM ]

After poking at it for a while, I discovered that the test in question isn't being executed in clojure.test-clojure.repl namespace, but in user. So replacing the failing test with:
(is (= (the-ns 'clojure.test-clojure.repl) ns))
also fails.

Which is very surprising to me. Is this the expected behavior? I could fix the test by doing the following:

(binding [*ns* (the-ns 'clojure.test-clojure.repl)]
(is (= (dir-fn 'clojure.string) (dir-fn 'str))))

But I don't want to mask an error, if this behavior is unexpected. Guidance please.

Comment by Alex Miller [ 12/Oct/15 7:57 PM ]

I don't want this to get applied as is so moving to incomplete for now.

Comment by Alex Miller [ 12/Oct/15 8:00 PM ]

You know this could be due to direct linking - calls inside the clojure code to other clojure code are now direct linked (static) calls as of 1.8.0-alpha3 and can't be redef'ed.

Comment by Alex Miller [ 12/Oct/15 8:03 PM ]

Yeah, if you turn direct linking off the test passes.

Comment by Jason Whitlark [ 13/Oct/15 1:06 PM ]

Fixed test to work with direct linking.

Comment by Jason Whitlark [ 10/Nov/15 12:58 AM ]

This is ready to go. Do I need to do anything to get it back in the queue?

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

We've missed the window for 1.8 but it's in the queue for 1.9.





[CLJ-1630] Destructuring allows multiple &-params Created: 31/Dec/14  Updated: 06/Sep/16  Resolved: 06/Sep/16

Status: Closed
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: Completed 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-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-1298] Add more type predicate fns to core Created: 21/Nov/13  Updated: 07/Jun/16  Resolved: 07/Jun/16

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

Type: Enhancement Priority: Critical
Reporter: Alex Fowler Assignee: Unassigned
Resolution: Completed Votes: 18
Labels: None

Approval: Ok

 Description   

Add more built-in type predicates:

1) Definitely missing: (atom? x), (ref? x), (deref? x), (named? x), (map-entry? x), (lazy-seq? x), (boolean?).
2) Very good to have: (throwable? x), (exception? x), (pattern? x).

The first group is especially important for writing cleaner code with core Clojure.



 Comments   
Comment by Alex Miller [ 21/Nov/13 8:42 AM ]

In general many of the existing predicates map to interfaces. I'm guessing these would map to checks on the following types:

atom? = Atom (final class)
ref? = IRef (interface)
deref? = IDeref (interface)
named? = Named (interface, despite no I prefix)
map-entry? = IMapEntry (interface)
lazy-seq? = LazySeq (final class)

throwable? = Throwable
exception? = Exception, but this seems less useful as it feels like the right answer when you likely actually want throwable?
pattern? = java.util.regex.Pattern

Comment by Alex Fowler [ 21/Nov/13 9:02 AM ]

Yes, they do, and sometimes the code has many checks like (instance? clojure.lang.Atom x). Ok, you can write a little function (atom? x) but it has either to be written in all relevant namespaces or required/referred there from some extra namespace. All this is just a burden. For example, we have predicates like (var? x) or (future? x) which too map to Java classes, but having them abbreviated often makes possible to write a cleaner code.

I feel the first group to be especially significant for it being about core Clojure concepts like atom and ref. Having to fall to manual Java classes check to work with them feels inorganic. Of course we can, but why then do we have (var? x), (fn? x) and other? Imagine, for example:

(cond
(var? x) (...)
(fn? x) (...)
(instance? clojure.lang.Atom x) (...)
(or (instance? clojure.lang.Named x) (instance? clojure.lang.LazySeq x)) (...))

vs

(cond
(var? x) (...)
(fn? x) (...)
(atom? x) (...)
(or (named? x) (lazy-seq? x)) (...))

The second group is too, essential since these concepts are fundamental for the platform (but you're right with the (exception? x) one).

Comment by Alex Fowler [ 22/Nov/13 6:35 AM ]

Also, obviously I missed the (boolean? x) predicate in the original post. Did not even guess it is absent too until I occasionally got into it today. Currently the most clean way we have is to do (or (true? x) (false? x)). Needles to say, it looks weird next to the present (integer? x) or (float? x).

Comment by Brandon Bloom [ 22/Jul/14 1:02 AM ]

Predicates for core types are also very useful for portability to CLJS.

Comment by Brandon Bloom [ 22/Jul/14 1:05 AM ]

I'd be happy to provide a patch for this, but I'd prefer universal interface support where possible. Therefore, this ticket, in my mind, is behind http://dev.clojure.org/jira/browse/CLJ-803 etc.

Comment by Alex Miller [ 22/Jul/14 6:12 AM ]

I don't think it's worth making a ticket for this until Rich has looked at it and determined which parts are wanted.

Comment by Alex Miller [ 02/Dec/14 4:33 PM ]

Someone asked about a boolean? predicate, so throwing this one on the list...

Comment by Reid McKenzie [ 02/Dec/14 4:51 PM ]

uuid? maybe. UUIDs have a bit of a strange position in that we have special printer handling for them built into core implying that they are intentionally part of Clojure, but there is no ->UUID constructor and no functions in core that operate on them so I could see this one being specifically declined.

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

This has been troubling me again with my first cljc project. So, I've added a whole bunch of tickets (with patches!) for individual predicates in both CLJ and CLJS.

Comment by Alex Miller [ 03/May/15 5:35 PM ]

As I said above, I don't want to mess with specific patches or tickets on this until Rich gets a look at this and we decide which stuff should and should not be included. So I'm going to ignore your other tickets for now...

Comment by Nicola Mometto [ 24/Nov/15 4:08 PM ]

map-entry? is included since 1.8

Comment by Alex Miller [ 07/Jun/16 11:36 AM ]

We have spent a significant amount of time looking at additional predicates for Clojure and a large commit was just added to master with many new ones in https://github.com/clojure/clojure/commit/58227c5de080110cb2ce5bc9f987d995a911b13e.

Those added were:

  • seqable? (also covered in separate ticket)
  • boolean?
  • long?, pos-long?, neg-long?, nat-long?
  • double?
  • bigdec?
  • ident? (like the named? proposed), simple-ident?, qualified-ident?
  • simple-symbol?, qualified-symbol?
  • simple-keyword?, qualified-keyword?
  • bytes? (for byte[])
  • indexed?
  • inst? (for Date, but backed by a protocol for extension)
  • uuid?
  • uri?

I'm closing this as I don't expect to add any more mentioned in the ticket.





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

Status: Closed
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: Completed Votes: 7
Labels: collections

Attachments: Text File 0001-CLJ-1242-equals-doesn-t-throw-on-sorted-collections.patch     Text File 0001-fix-for-CLJ-1242-tests.patch    
Patch: Code and Test
Approval: Ok

 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.

Proposed: 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)

Patch: 0001-CLJ-1242-equals-doesn-t-throw-on-sorted-collections.patch

Screened by: Alex Miller



 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." ?

Comment by Alex Miller [ 22/Sep/16 1:45 PM ]

I don't think this change is good in its current location - should reconsider alternative impl based on the proposed suggestion.

Comment by Nicola Mometto [ 07/Oct/16 3:31 PM ]

Updated patch to only handle equals/equiv

Comment by Alex Miller [ 11/Oct/16 10:46 PM ]

Rich: TreeSet extends AbstractSet and uses its equals() implementation - that impl checks for ClassCastException and returns null. http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/AbstractSet.java?av=f#96

Comment by Nicola Mometto [ 12/Oct/16 5:08 AM ]

updated patch to fix indentation





[CLJ-1224] Records do not cache hash like normal maps Created: 24/Jun/13  Updated: 07/Sep/16  Resolved: 07/Sep/16

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

Type: Enhancement Priority: Critical
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 21
Labels: defrecord, performance

Attachments: Text File 0001-cache-hasheq-and-hashCode-for-records.patch     Text File 0001-cache-hasheq-and-hashCode-for-records-v2.patch     Text File 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch     Text File 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records-v2.patch     Text File clj-1224-3.patch    
Patch: Code and Test
Approval: Ok

 Description   

Records do not cache their hash codes like normal Clojure maps, which affects their performance. This problem has been fixed in CLJS, but still affects JVM CLJ.

Approach: Cache hash values in record definitions, similar to maps.

Timings:

coll 1.9.0-master 1.9.0-master+patch
small record 90 ns 7 ns
big record 446 ns 8 ns
(defrecord R [a b])  ;; small
(def r  (R. (range 1e3) (range 1e3)))
(bench (hash r))
(defrecord R [a b c d e f g h i j])  ;; big
(def r (map->R (zipmap [:a :b :c :d :e :f :g :h :i :j] (repeat (range 1e3)))))
(bench (hash r))

Patch: clj-1224-3.patch

Screened by: Alex Miller

Also see: http://dev.clojure.org/jira/browse/CLJS-281



 Comments   
Comment by Nicola Mometto [ 14/Feb/14 5:46 PM ]

I want to point out that my patch breaks ABI compatibility.
A possible approach to avoid this would be to have 3 constructors instead of 2, I can write the patch to support this if desired.

Comment by Stuart Halloway [ 27/Jun/14 11:09 AM ]

The patch 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch is broken in at least two ways:

  • The fields __hash and __hasheq are adopted by new records created by .assoc and .without, which will cause those records to have incorrect (and likely colliding) hash values
  • The addition of the new fields breaks the promise of defrecord, which includes an N+2 constructor taking meta and extmap. With the patch, defrecords get an N+4 constructor letting callers pick hash codes.

I found these problems via the following reasoning:

  • Code has been touched near __extmap
  • Grep for all uses of __extmap and see what breaks
Comment by Nicola Mometto [ 27/Jun/14 2:53 PM ]

Patch 0001-cache-hasheq-and-hashCode-for-records.patch fixes both those issues, reintroducing the N+2 arity constructor

Comment by Alex Miller [ 27/Jun/14 4:08 PM ]

Questions addressed, back to Vetted.

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

All patches dated Jun 7 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 this patch.

Comment by Alex Miller [ 29/Aug/14 4:40 PM ]

Would be great to get this one updated as it's otherwise ready to screen.

Comment by Nicola Mometto [ 29/Aug/14 4:58 PM ]

Updated patch to apply to lastest master

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

1) hash and hasheq are stored as Objects, which seems kind of gross. It would be much better to store them as primitive longs and check whether they'd been calculated by comparing to 0. We still end up with a long -> int conversion but at least we'd avoid boxing.

2) assoc wrongly copies over the __hash and __hasheq to the new instance:

(defrecord R [a])
(def r (->R "abc"))
(hash r)                   ;; -1544829221
(hash (assoc r :a "def"))  ;; -1544829221

3) Needs some tests if they don't already exist:

  • with-meta on a record should not affect hashcode
  • modifying a record with assoc or dissoc should affect hashcode
  • maybe something else?

4) Needs some perf tests with a handful of example records (at least: 1 field, many fields, extmap, etc).

Nicola, I'm happy to let you continue to do dev on this patch with me doing the screening if you have time. Or if you don't have time, let me know and I (or someone else) can work on the dev parts. Would like to get this prepped and clean for 1.8.

Comment by Nicola Mometto [ 16/Jun/15 5:56 PM ]

Updated patch addresses the issues raised, will add some benchmarks later

Comment by Nicola Mometto [ 16/Jun/15 5:59 PM ]

Alex, regarding point 1, I stored __hash and __hasheq as ints rather than longs and compared to -1 rather than 0, to be consistent with how it's done elsewhere in the clojure impl

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

Looking at the bytecode for hashcode and hasheq, I had two questions:

1) the -1 there is a long and requires an upcast of the private field from int to long. I'm sure that's not a big deal, but wish there was a way to avoid it. I didn't try it but maybe (int -1) would help the compiler out?

2) I wonder whether something like this would perform better:

`(hasheq [this#] 
   (if (== -1 ~'__hasheq)
     (set! ~'__hasheq (int (bit-xor ~type-hash (clojure.lang.APersistentMap/mapHasheq this#)))))
   ~'__hasheq)

The common case will be a failed compare and then the field can be loaded and returned directly without any casting.

Comment by Nicola Mometto [ 17/Jun/15 11:54 AM ]

1- there's no Numbers.equiv(int, int) so even casting -1 to an int wouldn't solve this. a cast is always necessary. if we were to make hasheq a long, we'd need l2i in the return path, making hasheq an int we need an i2l in the comparison.
2- that doesn't remove any casting, it just replaces a load from the local variable stack with a field load:

;; current version
0: ldc2_w        #203                // long -1l
3: aload_0
4: getfield      #236                // Field __hasheq:I
7: i2l
8: lcmp
9: ifne          38
12: ldc2_w        #267                // long 1340417398l
15: aload_0
16: checkcast     #16                 // class clojure/lang/IPersistentMap
19: invokestatic  #274                // Method clojure/lang/APersistentMap.mapHasheq:(Lclojure/lang/IPersistentMap;)I
22: i2l
23: lxor
24: invokestatic  #278                // Method clojure/lang/RT.intCast:(J)I
27: istore_1
28: aload_0
29: iload_1
30: putfield      #236                // Field __hasheq:I
33: iload_1
34: goto          42
37: pop
38: aload_0
39: getfield      #236                // Field __hasheq:I
42: ireturn
;; your version
0: ldc2_w        #203                // long -1l
3: aload_0
4: getfield      #236                // Field __hasheq:I
7: i2l
8: lcmp
9: ifne          35
12: aload_0
13: ldc2_w        #267                // long 1340417398l
16: aload_0
17: checkcast     #16                 // class clojure/lang/IPersistentMap
20: invokestatic  #274                // Method clojure/lang/APersistentMap.mapHasheq:(Lclojure/lang/IPersistentMap;)I
23: i2l
24: lxor
25: invokestatic  #278                // Method clojure/lang/RT.intCast:(J)I
28: putfield      #236                // Field __hasheq:I
31: goto          37
34: pop
35: aconst_null
36: pop
37: aload_0
38: getfield      #236                // Field __hasheq:I
41: ireturn
Comment by Alex Miller [ 17/Jun/15 12:01 PM ]

Fair enough! Looks pretty good to me, still needs the perf numbers.

Comment by Nicola Mometto [ 17/Jun/15 1:00 PM ]
coll 1.7.0-RC2 1.7.0-RC2+patch
big record ~940ns ~10ns
small record ~150ns ~11ns
;; big record, 1.7.0-RC2
user=> (use 'criterium.core)
nil
user=> (defrecord R [a b c d e f g h i j])
user.R
user=> (def r (map->R (zipmap [:a :b :c :d :e :f :g :h :i :j] (repeat (range 1e3)))))
#'user/r
user=> (bench (hash r))
WARNING: Final GC required 1.291182176566658 % of runtime
Evaluation count : 63385020 in 60 samples of 1056417 calls.
             Execution time mean : 943.320293 ns
    Execution time std-deviation : 44.001842 ns
   Execution time lower quantile : 891.919381 ns ( 2.5%)
   Execution time upper quantile : 1.033894 µs (97.5%)
                   Overhead used : 1.980453 ns

;; big record, 1.7.0-RC2 + patch
user=> (defrecord R [a b c d e f g h i j])
user.R
user=> (def r (map->R (zipmap [:a :b :c :d :e :f :g :h :i :j] (repeat (range 1e3)))))
#'user/r
user=> (bench (hash r))
WARNING: Final GC required 1.0097162582088168 % of runtime
Evaluation count : 4820968380 in 60 samples of 80349473 calls.
             Execution time mean : 10.657581 ns
    Execution time std-deviation : 0.668011 ns
   Execution time lower quantile : 9.975656 ns ( 2.5%)
   Execution time upper quantile : 12.190471 ns (97.5%)
                   Overhead used : 2.235715 ns

;; small record 1.7.0-RC2
user=> (defrecord R [a b])
user.R
user=> (def r  (R. (range 1e3) (range 1e3)))
#'user/r
user=> (bench (hash r))
WARNING: Final GC required 1.456092401467115 % of runtime
Evaluation count : 423779160 in 60 samples of 7062986 calls.
             Execution time mean : 147.154359 ns
    Execution time std-deviation : 8.148340 ns
   Execution time lower quantile : 138.052054 ns ( 2.5%)
   Execution time upper quantile : 165.573489 ns (97.5%)
                   Overhead used : 1.629944 ns

;; small record 1.7.0-RC2+patch
user=> (defrecord R [a b])
user.R
user=> (def r  (R. (range 1e3) (range 1e3)))
#'user/r
user=>  (bench (hash r))
WARNING: Final GC required 1.720638384341818 % of runtime
Evaluation count : 4483195020 in 60 samples of 74719917 calls.
             Execution time mean : 11.696574 ns
    Execution time std-deviation : 0.506482 ns
   Execution time lower quantile : 10.982760 ns ( 2.5%)
   Execution time upper quantile : 12.836103 ns (97.5%)
                   Overhead used : 2.123801 ns
Comment by Alex Miller [ 17/Jun/15 3:36 PM ]

Screened for 1.8.

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

Note that using -1 for the uncomputed hash value can cause issues with transient lazily computed hash codes on serialization (CLJ-1766). In this case, the defrecord cached code is not transient so I don't think it's a problem, but something to be aware of. Using 0 would avoid this potential issue.

Comment by Rich Hickey [ 17/Jul/15 12:00 PM ]

So, why not use 0? You won't need initialization then either

Comment by Nicola Mometto [ 17/Jul/15 7:50 PM ]

Updated patch so that it applies on current master and changed the default hash value from -1 to 0.

Rich, we still need initialization since all the record ctors delegate to the ctor arity with explicit __hash and __hasheq, following the approach of the alt ctors for __extmap and __meta

Comment by Alex Miller [ 17/Jul/15 10:30 PM ]

Moving back to vetted for screening

Comment by Alex Miller [ 28/Jul/15 6:17 PM ]

Hey Nicola, two comments on the hasheq/hashcode impl:

1) I don't think there's any reason to use == in the check instead of =, and = seems better (I think the resulting bytecode is same either way though).

2) The generated bytecode in these cases will call getfield twice in the cached case (once for the check, and once for the return):

public int hasheq();
    Code:
       0: lconst_0      
       1: aload_0       
       2: getfield      #232                // Field __hasheq:I     ;; <-- HERE
       5: i2l           
       6: lcmp          
       7: ifne          36
      10: ldc2_w        #263                // long -989260517l
      13: aload_0       
      14: checkcast     #16                 // class clojure/lang/IPersistentMap
      17: invokestatic  #270                // Method clojure/lang/APersistentMap.mapHasheq:(Lclojure/lang/IPersistentMap;)I
      20: i2l           
      21: lxor          
      22: invokestatic  #274                // Method clojure/lang/RT.intCast:(J)I
      25: istore_1      
      26: aload_0       
      27: iload_1       
      28: putfield      #232                // Field __hasheq:I
      31: iload_1       
      32: goto          40
      35: pop           
      36: aload_0       
      37: getfield      #232                // Field __hasheq:I   ;; <-- HERE
      40: ireturn

Letting a local will avoid that:

`(hasheq [this#] (let [hv# ~'__hasheq]     ;; ADDED
                                  (if (= 0 hv#)           ;; USED
                                    (let [h# (int (bit-xor ~type-hash (clojure.lang.APersistentMap/mapHasheq this#)))]
                                      (set! ~'__hasheq h#)
                                      h#)
                                    hv#)))                ;; USED

Output bytecode:

public int hasheq();
    Code:
       0: aload_0       
       1: getfield      #227                // Field __hasheq:I
       4: istore_1      
       5: lconst_0      
       6: iload_1       
       7: i2l           
       8: lcmp          
       9: ifne          38
      12: ldc2_w        #258                // long -989260517l
      15: aload_0       
      16: checkcast     #16                 // class clojure/lang/IPersistentMap
      19: invokestatic  #265                // Method clojure/lang/APersistentMap.mapHasheq:(Lclojure/lang/IPersistentMap;)I
      22: i2l           
      23: lxor          
      24: invokestatic  #269                // Method clojure/lang/RT.intCast:(J)I
      27: istore_2      
      28: aload_0       
      29: iload_2       
      30: putfield      #227                // Field __hasheq:I
      33: iload_2       
      34: goto          39
      37: pop           
      38: iload_1       
      39: ireturn

For me, this was about 2% faster in bench too.

Comment by Alex Miller [ 28/Jul/15 6:18 PM ]

Equivalent change in hashCode too.

Comment by Nicola Mometto [ 29/Jul/15 4:06 AM ]

Updated patch takes into account Alex's last notes

Comment by Alex Miller [ 06/Sep/16 2:10 PM ]

Added clj-1224-3.patch - identical to prior but uses zero? rather than = 0 at Rich's request. Attribution retained.





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

Status: Closed
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: Completed 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-401] Add seqable? predicate Created: 13/Jul/10  Updated: 07/Jun/16  Resolved: 07/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: Unassigned
Resolution: Completed Votes: 7
Labels: None

Approval: Ok

 Description   

Many people have found a need for this function and one exists in clojure.core.incubator that is sometimes used and/or copied elsewhere:

https://github.com/clojure/core.incubator/blob/master/src/main/clojure/clojure/core/incubator.clj#L83

This predicate would be valuable to have as it is not a simple check on Seqable since RT.seq() covers a number of additional cases. Alternatively, there could be a protocol for this that could be extended to both Seqable as well as other supported Java use cases turning this into a satisfies? check.

Old prior discussion (although this also comes up regularly on #clojure):



 Comments   
Comment by Assembla Importer [ 24/Aug/10 9:19 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/401

Comment by Jeremy Heiler [ 26/Jul/14 5:37 PM ]

A reference to the implementation in contrib: https://github.com/clojure/clojure-contrib/blob/master/modules/core/src/main/clojure/clojure/contrib/core.clj#L78

It seems like that the only thing that is inconsistent with RT.seqFrom is that seqable? checks for String instead of CharSequence.

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

In the proposed patch referenced in the ticket above, if seqable? could be used in place of sequential? flatten could be more powerful and work with maps/sets/java collections. Here's how it would look:

(defn flatten [coll] 
  (lazy-seq 
    (when-let [coll (seq coll)] 
      (let [x (first coll)] 
        (if (seqable? x) 
          (concat (flatten x) (flatten (next coll))) 
          (cons x (flatten (next coll))))))))

And an example:

user=> (flatten #{1 2 3 #{4 5 {6 {7 [8 9 10 #{11 12 (java.util.ArrayList. [13 14 15]) (int-array [16 17 18])}]}}}}) 
(1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18)
Comment by Alex Miller [ 07/Jun/16 11:31 AM ]

This was just added to master in https://github.com/clojure/clojure/commit/58227c5de080110cb2ce5bc9f987d995a911b13e and will be in 1.9.0-alpha5.





Generated at Tue Dec 06 10:12:12 CST 2016 using JIRA 4.4#649-r158309.