<< Back to previous view

[CLJ-2311] Spec generator override won't work on multi-spec dispatch key Created: 12/Jan/18  Updated: 12/Jan/18

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

Type: Defect Priority: Minor
Reporter: Andreas Liljeqvist Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: generator, spec
Environment:

[org.clojure/clojure "1.9.0"]
[org.clojure/spec.alpha "0.1.143"]


Attachments: File multi_spec_bug.clj    
Approval: Triaged

 Description   

Overriding generator is not used in the final result for multi-spec dispatch key.

Complete example is attached.
Of particular interest is that the an invalid custom generator can be shown to introduce a failure in validation.
The default generator is seen in the exercised data when using a valid generator.






[CLJ-2309] Readable keyword validation Created: 05/Jan/18  Updated: 08/Jan/18

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

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

Approval: Triaged

 Description   

A common question is why it is possible to programatically create keyword instances that cannot be read (https://clojure.org/guides/faq#unreadable_keywords).

a) There are many places where programmatic keywords are perfectly valid
b) It is undesirable to validate all keywords on construction due to the perf hit

However, there are use cases for creating keywords that you expect to be safe for pr/read roundtripping and in those cases it would be useful to have either a variant of `keyword` that was more restrictive and would throw on invalid keywords OR a predicate that could tell you whether a string would be a readable keyword.



 Comments   
Comment by Phill Wolf [ 08/Jan/18 7:56 PM ]

1. Keywords are too often prepared far away from the eventual pr.

2. It is impressive how seldom deviant keywords are a problem this way. The fix should be proportionate to the problem.

3. Irreadable keywords are explicitly not the problem. The problem belongs to pr and read. The immediate problem is that pr silently fails to print readably while print-readably.

3b. This problem with pr/read might be temporary.

4. Adding a special predicate for readable keywords presupposes that you can predict whether someone will call later pr. I think such a prediction would usually be baseless without mingling concerns.

5. pr could increment irreadable-forms-printed when emitting a non-readable keyword while print-readably. I/O is expensive anyway.

6. If pr/read someday gained the ability to round-trip all keywords, pr would still have to test a keyword to choose an output format for it. Thus a check installed today could be a first step, not a temporary measure.

7. By comparison, a program littered with "readable-keyword?" predicates would stay littered forever – and the predicate check might not even be necessary anymore.

8. Would "readable-keyword?" match the reader's behavior, the reader's documentation, EDN, or some conservative common subset? related: CLJ-1527 "Clarify and align valid symbol and keyword rules for Clojure (and edn)", CLJ-1530 "Make foo/bar/baz unreadable".

9. Would every spec writer agonize about whether to spec a keyword as "keyword?" or "readable-keyword?" – and then always choose "readable-keyword?" just in case someone might ever someday eventually decide to serialize data?.. thereby cluttering the program and obscuring concerns.

10. Who would test irreadable-forms-printed? An "apologetic", non-solution resolution could get quite complicated.

11. Perhaps the cure would be worse than the disease. Why not improve pr/read instead: eliminate the problem instead of adding features to work around it?

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

The problem here is specifically about keywords created from either user input or other data outside your control (arbitrary keys from json input for example). When you need to verify this property you are accepting inputs, converting them to keywords, and then later expect to print that data out. I have talked through this with many many people and when people ask about it, they know they are in this situation. Having printing fail or report way down the line is not helpful. The problem is avoiding the creation of the non-roundtrippable data in the first place (and either not accepting it or escaping it at that point).

That said, another possible solution is to add an escaping mechanisms for literal symbols and keywords. We've done some design work on this in the past and ended up shelving it at the time but that's still another possible option.

This is not a high priority issue right now, but I felt it was useful to leave this ticket here to capture the idea.





[CLJ-2297] PersistentHashMap leaks memory when keys are removed with `without` Created: 20/Dec/17  Updated: 20/Dec/17

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

Type: Defect Priority: Critical
Reporter: Ben Bader Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections

Attachments: File demo-phm-leak.clj     Text File fix-bitmapnode-without.patch    
Approval: Triaged

 Description   

The problem is in `PersistentHashMap.BitmapNode#without(Object)`; when the last value is removed, an empty BitmapNode is returned instead of null. This has knock-on effects in large maps that have interior ArrayNodes, which themselves are not preserved.

Attached is a test script that demonstrates the issue, by measuring heap usage, adding a large number of entries to a map, removing those keys one-by-one, then comparing heap usage afterwards. The output, shows the average amount of heap allocated for an empty map that has had 100,000 entries added then removed:

❯ java -jar clojure.jar demo-phm-leak.clj
Avg. heap:8656352/5 bytes
Avg. heap:8656632/5 bytes
Avg. heap:8654664/5 bytes
Avg. heap:8656884/5 bytes
Avg. heap:1731156 bytes

This was from my a Macbook Pro, running a self-made Clojure built from master, using java 1.8.0_65. The variable sizes are due to the fact that the shape of the PHM tree depends on the insertion order of keys, which is randomized in this script.



 Comments   
Comment by Ben Bader [ 20/Dec/17 1:33 PM ]

This patch fixes the leak by changing `BitmapNode#without(Object)` such that, when there are no other values in the node, the method returns `null` instead of an empty BitmapNode.

Output from the demo script, from a Clojure build incorporating the patch:

~/Development/clojure collapse-empty-mapnodes* 16s
❯ java -jar clojure.jar demo-phm-leak.clj
Avg. heap:64 bytes
Avg. heap:64 bytes
Avg. heap:64 bytes
Avg. heap:64 bytes
Avg. heap:64 bytes

Comment by Ben Bader [ 20/Dec/17 1:54 PM ]

Note that this patch doesn't have a similar change for the transient overload of `without`; because it relies on a helper method `editAndRemovePair` that correctly handles the empty case, that method doesn't have this bug.

Comment by Alex Miller [ 20/Dec/17 2:17 PM ]

Thanks for the report!

Comment by Alex Miller [ 20/Dec/17 2:23 PM ]

I assume this came out of some actual usage with surprising behavior?

Comment by Ben Bader [ 20/Dec/17 4:44 PM ]

It came up when I was profiling a service that uses maps in agents to cache things; overall memory usage seemed high, and I got curious. Hard to say whether this would ever noticeably impact a production system, given that this behavior has been in place for nine years!

Comment by Andy Fingerhut [ 20/Dec/17 7:34 PM ]

Cool find. It looks like an average of 1.7 bytes of unreclaimed space per 'without' operation in your test case, so nice to improve upon, but perhaps not something people would notice due to it causing problems very often.





[CLJ-2284] Incorrect bytecode generated for static methods on interfaces Created: 09/Dec/17  Updated: 24/Dec/17

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

Type: Defect Priority: Major
Reporter: Zach Tellman Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: asm, compiler, java19
Environment:

JDK 9 or higher


Attachments: Text File 0001-unbundle-ASM.patch     Text File 0002-update-classfile-version-to-9.patch     Text File 0003-CLJ-2284.patch    
Approval: Triaged

 Description   

A reproduction of the failure can be found here: https://github.com/ztellman/java9-failure.



 Comments   
Comment by Ghadi Shayban [ 09/Dec/17 11:08 PM ]

Static or default interface method invocations are not supported when the bytecode emitted is < 1.8 bytecode (Clojure currently emits 1.6 bytecode). I'm mildly surprised this compiled.

Comment by David Bürgin [ 21/Dec/17 12:54 PM ]

Not sure Ghadi’s assertion is correct? No new bytecodes were introduced for static and default interface methods. Or at least I have been using JDK 8 utilities like CharSequence.codePoints() and Comparator.reverseOrder() in Clojure for a long time. (And if this usage were not supported today that would seem like a critical issue.)

Comment by Zach Tellman [ 22/Dec/17 9:08 PM ]

Yeah, I'd expect default interface methods to just be filled in on classes which don't provide their own implementation, and empirically they don't seem to cause any issues, but I'm not an authority on JVM bytecode.

Comment by Nicola Mometto [ 23/Dec/17 2:10 PM ]

while it is true that no bytecodes were introduced, the JVM spec mandates that invocations to static interface methods refer to an InterfaceMethodref entry in the constant pool rather than simply to a Methodref – as to why this worked in jvm 1.8 and fails with a verify error in jvm 9 , I can only assume the bytecode verifier became stricter since version 9

See https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.4.3.3, describing the resolution rules for Methodrefs:

When resolving a method reference:

    If C is an interface, method resolution throws an IncompatibleClassChangeError.
Comment by Nicola Mometto [ 23/Dec/17 2:21 PM ]

Indeed here's the openjdk ticket tightening bytecode verification to adhere to the JVM spec: https://bugs.openjdk.java.net/browse/JDK-8145148
According to the last comment on that ticket, upgrading the ASM version provided with clojure to 5.1 should fix this

Comment by Nicola Mometto [ 24/Dec/17 12:31 PM ]

The following 3 patches work to fix this problem, I've kept them split for now so that we can be evaluated separately, but in order to fix this all three are required.

1st patch simply updates the version of ASM that clojure uses to 6.0, removes the bundled ASM and adds ASM as a separate dependency. If this is not desirable we could just update the bundled version of ASM but it seems like now that clojure requires external deps, this is up for debate.

2nd patch updates the classfile version that clojure emits from 1_5 to 9 so that ASM can emit interface method refs, because of changes in bytecode verification since v1.5, several additional changes were needed:
1- it's not possible anymore to assign final fields from methods other than <clinit>, so it was necessary to mark the const__ fields as non static as they are assigned by init__n methods
2- stack map frames are not optional anymore so we need to tell ASM to compute them for us
3- a custom ClassWriter overriding getCommonSuperClass has been implemented, using DCL to load classes and short circuiting on classes that are being defined (as per that method's javadoc)

finally the third patch addresses this issue by emitting `invokeStatic` through the new `visitmethodInsn` arity that supports interface methods

Comment by Nicola Mometto [ 24/Dec/17 12:41 PM ]

Note that I'm not proposing the 3 patches as they are as a fix for this, given that bumping classfile version means discontinuing support for running clojure on java versions earlier than what's specified, which is undesiderable (I've bumped it to V9 but what's needed here should be just V1_8, but still).

That said, if we want to fix this, we definitely need the fixes I've put in those patches and we likely also want to conditionally emit different classfile versions (e.g. defaulting to 1_5 but bumping it to a higher version for specific classfiles when needed)





[CLJ-2283] doseq should return nil with no collections Created: 09/Dec/17  Updated: 15/Dec/17

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

Type: Defect Priority: Major
Reporter: Ambrose Bonnaire-Sergeant Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File CLJ-2283.patch     Text File CLJ-2283-test.patch     Text File CLJ-2283-test-revised.patch    
Approval: Triaged

 Description   

According to the docstring for doseq, the following should return `nil`.

user> (doseq [] "not nil")
"not nil"


 Comments   
Comment by Michael Zavarella [ 13/Dec/17 3:47 PM ]

I think this fixes this issue.

Theres a `(do ...)` that runs if `(seq exprs)` is nil but the return of that `do` isn't necessarily nil if your body is something that doesn't return nil.

Comment by Mike Fikes [ 14/Dec/17 2:12 PM ]

It is odd that doseq even has code for this case given that the docstring indicates the bindings and filterings are as provided by "for." Since for requires one or more binding forms, it raises the question of whether (doseq [] "not nil") is a valid program.

Comment by Alex Miller [ 14/Dec/17 2:58 PM ]

Test?

Comment by Michael Zavarella [ 15/Dec/17 5:34 PM ]

Here's a bit of tests! I think these are suitable for the use case of `doseq`. I just went with similar tests to `dotimes` since they're so similar.

Comment by Michael Zavarella [ 15/Dec/17 5:36 PM ]

I removed the `println` from the original tests in favor of `identity`. It seemed more appropriate so you don't have a random 'foo' in the test print-out.





[CLJ-2275] case fails for vectors with negative numbers Created: 27/Nov/17  Updated: 27/Nov/17

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

Type: Defect Priority: Major
Reporter: Max Lorenz Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: case
Environment:

OSX


Attachments: Text File 0001-switch-to-hasheq-for-case.patch    
Patch: Code and Test
Approval: Triaged

 Description   
user=> (case -1 -1 true false)
true
user=> (case [-1] [-1] true false)
true
user=> (case (int -1) -1 true false)
true
user=> (case [(int 1)] [1] true false)
true
user=> (case [(int -1)] [-1] true false)
false

The last case should return true like the other,

Real life example that triggered this:

(case [">" (compare 2 3)]
  [">" -1] true
  false) ;; false?

Explaination: This is caused by `case` using `hashCode` instead of hashEq for hash comparisons (when not wrapped in a vector, the comparison is direct) and the fact that negative integers and negative longs hash differently while positive ones hash identical.
Porposal: Make `case` use hasheq instead of hashCode

Patch: 0001-switch-to-hasheq-for-case.patch



 Comments   
Comment by Nicola Mometto [ 27/Nov/17 10:38 AM ]

Patch is a straightforward search and replace, apart from line 30 of the diff which is quite tricky: I believe using `h` there was a bug in the previous impl that never survaced because hashCode on positive integers is idempotent. But since hashEq isn't double hashing would cause collision nodes never to match.





[CLJ-2274] Line numbers in stack trace are wrong when type hints satisfaction fails Created: 26/Nov/17  Updated: 27/Nov/17

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

Type: Defect Priority: Minor
Reporter: Stijn Seghers Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs
Environment:

I only tested this on macOS 10.13.1 with both Clojure 1.8.0 and 1.9.0-RC1.


Approval: Triaged

 Description   

When I run the following file

example.clj
(defn f [^double x] x)

(defn g []
  (println)
  (f nil))

(g)

I get the following stack trace

Exception in thread "main" java.lang.NullPointerException, compiling:(/.../example.clj:7:1)
	at clojure.lang.Compiler.load(Compiler.java:7526)
	at clojure.lang.Compiler.loadFile(Compiler.java:7452)
	at clojure.main$load_script.invokeStatic(main.clj:278)
	at clojure.main$script_opt.invokeStatic(main.clj:338)
	at clojure.main$script_opt.invoke(main.clj:333)
	at clojure.main$main.invokeStatic(main.clj:424)
	at clojure.main$main.doInvoke(main.clj:387)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.lang.Var.applyTo(Var.java:702)
	at clojure.main.main(main.java:37)
Caused by: java.lang.NullPointerException
	at clojure.lang.RT.doubleCast(RT.java:1348)
	at user$g.invokeStatic(example.clj:4)
	at user$g.invoke(example.clj:3)
	at user$eval147.invokeStatic(example.clj:7)
	at user$eval147.invoke(example.clj:7)
	at clojure.lang.Compiler.eval(Compiler.java:7062)
	at clojure.lang.Compiler.load(Compiler.java:7514)
	... 9 more

The line at user$g.invokeStatic(example.clj:4) here is quite misleading, because the error happens at line 5.






[CLJ-2273] Add original 'assert' form to explain-data for s/assert Created: 21/Nov/17  Updated: 27/Nov/17

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

Type: Enhancement Priority: Major
Reporter: Ben Brinckerhoff Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

org.clojure/spec.alpha "0.1.143"


Approval: Triaged

 Description   

Using s/assert instead of assert has advantages: namely a specific reason why the data failed to meet the spec. But it also has disadvantages: the error message does not contain the assertion code the user wrote, so it's harder to see which assertion failed.

I believe we could have the best of both world if s/assert contained the original "assert" code - that way the error message could (optionally) print out this information.

(require '[clojure.spec.alpha :as s])
  (s/check-asserts true)

  (let [x 1]
    (s/assert string? x))

  ;; Spec assertion failed val: 1 fails predicate:
  ;; :clojure.spec.alpha/unknown

  (let [x 1]
    (assert (string? x)))

  ;; Assert failed: (string? x)





[CLJ-2271] "caller" information missing in explain-data during macro instrumentation failure Created: 19/Nov/17  Updated: 27/Nov/17

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

Type: Defect Priority: Major
Reporter: Ben Brinckerhoff Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting, spec
Environment:

org.clojure/spec.alpha "0.1.143"


Approval: Triaged

 Description   

When there is a instrumentation failure for a function, the explain-data includes "caller" information. However, this information is missing if the instrumentation failure is for a macro.

This comment has led me to believe that the intended behavior is for explain-data to contain this info, so third-party error printers can display it.

In the repro below, I'm setting up a custom printer just to capture the raw explain-data (it's not a useful printer, just a means to show what is happenening)

Repro:

(require '[clojure.spec.alpha :as s])
  (require '[clojure.spec.test.alpha :as st])
  (require '[clojure.specs.alpha :as s])


  (s/fdef my-fn
          :args (s/cat :x int?))
  (defn my-fn [x]
    x)

  (s/fdef my-macro
          :args (s/cat :x int?))
  (defmacro my-macro [x]
    x)

  (st/instrument)
  (def !ed (atom nil))
  (set! s/*explain-out* (fn [ed]
                          (reset! !ed ed)))
  (my-fn "")
  @!ed
  ;; {:clojure.spec.alpha/problems [{:path [:args :x], :pred clojure.core/int?, :val "", :via [], :in [0]}], :clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__2436 0x72029b0e "clojure.spec.alpha$regex_spec_impl$reify__2436@72029b0e"], :clojure.spec.alpha/value (""), :clojure.spec.alpha/args (""), :clojure.spec.alpha/failure :instrument, :clojure.spec.test.alpha/caller {:file "form-init8333540581183382896.clj", :line 548, :var-scope expound.alpha/eval27394}}

  ;; ^--- Note there is an entry for :clojure.spec.test.alpha/caller

  (my-macro "")
  @!ed

  ;; #:clojure.spec.alpha{:problems [{:path [:args :x], :pred clojure.core/int?, :val "", :via [], :in [0]}], :spec #object[clojure.spec.alpha$regex_spec_impl$reify__2436 0x479a6a73 "clojure.spec.alpha$regex_spec_impl$reify__2436@479a6a73"], :value (""), :args ("")}

  ;; ^--- No caller information


 Comments   
Comment by Alex Miller [ 27/Nov/17 8:39 AM ]

You can't instrument a macro, so that part of the ticket doesn't make sense as written. But I expect you mean the spec check during macro expansion.

In the macro check case, the caller info is known by the compiler and included in the wrapper CompilerException. I suppose that info could be passed into s/macroexpand-check from the Compiler and possibly produce similar results as with instrumented function calls.





[CLJ-2269] definterface seems not to resolve imported classes in type hints Created: 16/Nov/17  Updated: 27/Nov/17

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

Type: Defect Priority: Minor
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop, typehints
Environment:

Java 1.8.0_152
Windows 10


Approval: Triaged

 Description   

For example:

user=> (import java.util.Map)
java.util.Map
user=> (definterface Foo (^void foo [^Map map]))
user.Foo
user=> (deftype Bar [] Foo (foo [this m]))
CompilerException java.lang.NoClassDefFoundError: java/lang/Map, compiling:(NO_SOURCE_PATH:3:1)
user=> (definterface Foo2 (^void foo2 [^java.util.Map map]))
user.Foo2
user=> (deftype Bar2 [] Foo2 (foo2 [this m]))
user.Bar2

The attempt to type-hint with just Map fails; you have to use java.util.Map to get a usable interface definition.






[CLJ-2268] Spec asserts set : field incorrectly in explain-data Created: 15/Nov/17  Updated: 16/Nov/17

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

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

[org.clojure/spec.alpha "0.1.143"] [org.clojure/clojure "1.9.0-beta3"]


Approval: Triaged

 Description   

I would expect that "explain-data" should contain the same "via" entry, regardless of whether it is returned from a call to s/explain-data or if it passed to the printer during an assertion failure.

Repro:

(s/check-asserts true)
  
  (s/def :example/name string?)
  (def !ed (atom nil))


  (s/explain-data :example/name 1)
  ;; #:clojure.spec.alpha{:problems [{:path [], :pred clojure.core/string?, :val 1, :via [:example/name], :in []}], :spec :example/name, :value 1}


  (try
    (binding [s/*explain-out* (fn [ed]
                                (reset! !ed ed)
                                "captured")]
      (s/assert :example/name 1))
    (catch Exception e))

  @!ed
  ;; #:clojure.spec.alpha{:problems [{:path [], :pred clojure.core/string?, :val 1, :via [], :in []}], :spec :example/name, :value 1, :failure :assertion-failed}

Expected: The ":via" entries in both cases should the same
Actual: The ":via" entry is empty in the explain-data that is passed to the printer during the assertion failure.






[CLJ-2264] fspec conform function validation can't access outer gen overrides Created: 08/Nov/17  Updated: 08/Nov/17

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 Description   

Given a situation like this, exercising an fspec will fail:

(def uuid-regex #"^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$")

(s/def ::name string?)
(s/def ::age pos-int?)
(s/def ::id (s/and string? #(re-matches uuid-regex %)))
(s/def ::person (s/keys :req-un [::name ::age ::id]))

;; works - ::id gen override produces valid ids
(s/exercise ::person 1 {::id (fn [] (gen/fmap str (gen/uuid)))})

;; fails - ::id gen override isn't being used
(s/exercise 
  (s/fspec :args (s/cat :p ::person) :ret int?) 
  10 
  {::id #(gen/fmap str (gen/uuid))})
Couldn't satisfy such-that predicate after 100 tries.

Problem: exercise will generate a function that validates the args and returns a gen int. When exercise then conforms that generated fspec function, it calls conform on the fspec-impl. fspec-impl conform* calls validate-function. validate-function runs quick-check on the generated function but conform* and validate-function do not have access to the generator overrides specified in exercise. This same problem also happens if you run stest/check on anything using the fspec (like an fdef with an fspec arg).






[CLJ-2263] When calling a multi method with the wrong number of arguments, the error message could be better. Created: 07/Nov/17  Updated: 09/Nov/17

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

Type: Enhancement Priority: Major
Reporter: Erik Assum Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

Approval: Triaged

 Description   
09:43 $ clj
Clojure 1.8.0
(defmulti foo :bar)
(defmethod foo :qix [quux znoot] (println 'hi))
#'user/foo
user=> #object[clojure.lang.MultiFn 0x205d38da "clojure.lang.MultiFn@205d38da"]
user=> (foo {:bar :qix})
ArityException Wrong number of args (1) passed to: user/eval5/fn--6  clojure.lang.AFn.throwArity (AFn.java:429)
user=>

It is an implementation detail that multi methods are implemented via anonymous functions. I would expect the error message to at least contain the name of the function that failed, in this case it should have been something like

ArityException Wrong number of args (1) passed to: user/eval5/foo--6  clojure.lang.AFn.throwArity (AFn.java:429)

Several approaches can be taken here:

The first (and simplest) is to change the definition of defmethod so that the anonymous function gets a name.
This leads to an error message like:

user=> (defmulti foo :bar)
(defmethod foo :qix [quux znoot] (println 'hi))
(foo {:bar :qix})#'user/foo
user=> #object[clojure.lang.MultiFn 0x4e928fbf "clojure.lang.MultiFn@4e928fbf"]
user=>
ArityException Wrong number of args (1) passed to: user/eval5/foo--6  clojure.lang.AFn.throwArity (AFn.java:429)
user=>

In addition to this, one could modify Compiler.java to look for the calls to "addMethod"

String prefix = "eval";
if (RT.count(form) > 2) {
   Object third = RT.nth(form, 2);
   if (third != null &&
       "clojure.core/addMethod".equals(third.toString()))
      prefix = "multi_fn";
}
ObjExpr fexpr = (ObjExpr) analyze(C.EXPRESSION, RT.list(FN, PersistentVector.EMPTY, form), prefix + RT.nextID());

which would give us error messages like

ArityException Wrong number of args (1) passed to: user/multi-fn5/foo--6  clojure.lang.AFn.throwArity (AFn.java:441)


 Comments   
Comment by Alex Miller [ 09/Nov/17 9:19 AM ]

Interestingly, it is actually possible to give defmethods names that get included in their class names. The arglist for defmethod is: ([multifn dispatch-val & fn-tail]) where fn-tail is the tail arguments as passed to fn, which includes an optional name.

So you can do this:

(defmulti foo class)
;; create defmethod with a name HI:
(defmethod foo String HI [x] (throw (Exception. (str "hi " x))))

;; Invoke:
(foo "bleh")
Exception hi bleh  user/eval1248/HI--1249 (form-init1348837216879020682.clj:1)

Note the HI in the class name. This doesn't address everything above but it is a helpful debugging trick.





[CLJ-2261] dot form silently drops additional (invalid) args Created: 05/Nov/17  Updated: 06/Nov/17

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

Type: Defect Priority: Trivial
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, interop

Approval: Triaged

 Description   

If the second argument to . is a seq? every additional arg is silently dropped. In CLJS this throws a compile error.

(. "xyz" (substring 1) (throw :bug?))
=> "yz"

Might be neat to at least get a warning since it's not immediately obvious when accidentally using . instead of ...






[CLJ-2258] When fspec fails, meaning of ":val" is different than in normal spec failure Created: 29/Oct/17  Updated: 30/Oct/17

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

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

org.clojure/spec.alpha "0.1.134"


Approval: Triaged

 Description   

Context:

When data fails to conform to a spec, the ":val" of a problem points to the non-conforming data. This has the nice property that the ":val" (for each problem) will exist somewhere within the ":value" (for the entire "explain-data" structure). For example:

(require '[clojure.spec.alpha :as s])

(s/explain-data
 (s/coll-of int?)
 [1 2 :a])
  
;; #:clojure.spec.alpha{:problems ({:path [], :pred int?, :val :a, :via [], :in [2]}), :spec #object[clojure.spec.alpha$every_impl$reify__934 0x1b235c3e "clojure.spec.alpha$every_impl$reify__934@1b235c3e"], :value [1 2 :a]}

This interpretation of ":val" doesn't seem to apply when a function fails to conform to an fspec spec.

Repro:

(require '[clojure.spec.alpha :as s])

(s/explain-data
 (s/coll-of (s/fspec
             :args (s/cat :x int?)))
 [(fn [%] (/ 1 %))])

;; #:clojure.spec.alpha{:problems ({:path [], :pred (apply fn), :val (0), :reason "Divide by zero", :via [], :in [0]}), :spec #object[clojure.spec.alpha$every_impl$reify__934 0x420c3355 "clojure.spec.alpha$every_impl$reify__934@420c3355"], :value [#function[expound.alpha/eval28876/fn--28880]]}

Expected: In order to be consistent with the way "val" normally works, "val" should be the anonymous function. While the function arguments are very useful, perhaps they could be associated with a different key?
Actual: "val" contains the arguments to the function, so it's no longer true that the "val" exists within the "value".



 Comments   
Comment by Ben Brinckerhoff [ 29/Oct/17 3:04 PM ]

This also makes the error messages misleading:

(s/explain
 (s/coll-of (s/fspec
             :args (s/cat :x int?)))
 [(fn [%] (/ 1 %))])

;; In: [0] val: (0) fails predicate: (apply fn),  Divide by zero

because it seems to indicate that the args are failing to conform, whereas the issue is that the function itself is not conforming.

Comment by Alex Miller [ 29/Oct/17 7:25 PM ]

Please don't set the fix version - core team will do this when it's targeted to a release. This won't be addressed for 1.9.

Comment by Ben Brinckerhoff [ 30/Oct/17 9:06 AM ]

Sorry about that - I'll be sure to avoid setting that field in the future.

A little more context: there are cases where the "in" for a problem is tricky to interpret (https://dev.clojure.org/jira/browse/CLJ-2192). In Expound, I have use a heuristic to figure out how the "in" path should work, but that heuristic depends on the "val" existing within the "value". Due to this issue, the heuristic doesn't work, which means I can't reliably report the spec error. See https://github.com/bhb/expound/issues/41





[CLJ-2255] When fspec spec fails due to return value, explain-data should contain args Created: 24/Oct/17  Updated: 25/Oct/17

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

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

org.clojure/spec.alpha "0.1.134"


Approval: Triaged

 Description   

Repro:

(require '[clojure.spec.alpha :as s])
(s/def :fspec-test/plus (s/fspec
                         :args (s/cat :x int? :y pos-int?)
                         :ret pos-int?))

(defn my-plus [x y]
  (+ x y))

(s/explain-data :fspec-test/plus my-plus)

Actual:

;; #:clojure.spec.alpha{:problems [{:path [:ret], :pred clojure.core/pos-int?, :val 0, :via [:fspec-test/plus], :in []}], :spec :fspec-test/plus, :value #function[expound.alpha-test/my-plus]}

Expected: I would expect the explain-data to contain the args that produced the invalid return value, so I can reproduce. Note that if my function throws an exception, the explain data will contain the args e.g.

(s/def :fspec-test/plus (s/fspec
                         :args (s/cat :x int? :y pos-int?)))
(defn my-plus [x y]
  (assert (pos? (+ x y))))

(s/explain-data :fspec-test/plus my-plus)
;; #:clojure.spec.alpha{:problems [{:path [], :pred (apply fn), :val (-1 1), :reason "Assert failed: (pos? (+ x y))", :via [:fspec-test/plus], :in []}], :spec :fspec-test/plus, :value #function[expound.alpha-test/my-plus]}





[CLJ-2253] slurp doesn't properly handle URLs that contain authentication information Created: 19/Oct/17  Updated: 23/Nov/17

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

Type: Enhancement Priority: Minor
Reporter: Peter Monks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: io
Environment:

n/a


Attachments: Text File 0001-CLJ-2253-Add-support-for-slurping-urls-with-authenti.patch     Text File 0002-CLJ-2253-Add-support-for-slurping-urls-with-authenti.patch    
Patch: Code
Approval: Triaged

 Description   

Because of limitations in the underlying Java classes [1], clojure.core/slurp doesn't support URLs that contain authentication information i.e. https://user:password@hostname.domain/path/to/resource. It would be great if slurp did support this (e.g. by using Java's UrlConnection class, as suggested in [1]), so that application code doesn't have to include special cases for different types of input string that will be sent to slurp.

[1] https://stackoverflow.com/questions/496651/connecting-to-remote-url-which-requires-authentication-using-java

Approach: Check to see if url contains userInfo, if so, open a connection, set authorisation on it and return its input stream. Otherwise call `openStream` on the url as before
Limitations: This approach uses java.xml.bind.DatatypeConverter, which is known to not work with java9



 Comments   
Comment by Alex Miller [ 19/Oct/17 2:38 PM ]

Re the note, this seems like a non-starter then?

Comment by Peter Monks [ 19/Oct/17 2:55 PM ]

What's the minimum version of the JVM that Clojure supports? Java 1.8 added java.util.Base64, and that may be a better choice for the encoding step (assuming the minimum supported JVM version is 1.8).

Alternatively, it seems like some kind of version-dependent "conditional compilation" would meet all cases - use java.xml.bind.DatatypeConverter for JVM <= 1.7, and use java.util.Base64 for JVM >= 1.8. Apologies for my unfamiliarity with the core Clojure codebase, but is there JVM-version-specific logic elsewhere that I could look at to see how this might be done?

Comment by Erik Assum [ 19/Oct/17 3:26 PM ]

I'd really appreciate some pointers to how to solve this, since Base64 encoding is needed. Could an option be to copy
https://github.com/clojure/data.codec/blob/master/src/main/clojure/clojure/data/codec/base64.clj
to a ns in clojure.core?

There is also http://www.docjar.com/html/api/java/util/prefs/Base64.java.html, which of course is package scoped.
I guess we could use reflection to get to this, but feels hacky.

Or should we just wait until java 8 is the lowest supported java platform?

Comment by Peter Monks [ 19/Oct/17 7:48 PM ]

Posting this may be a career limiting move, but this seems to work (tested on JDK 9 as-is, and on JDK 1.8 as-is as well as with the first clause in the or commented out to ensure both versions of the method got tested):

(let [jvm-version (System/getProperty "java.version")]
  (if (or (clojure.string/starts-with? jvm-version "1.8")
          (clojure.string/starts-with? jvm-version "9"))
    (eval
      (read-string
      "(defn- base64-encode
         [s]
         (.encodeToString (java.util.Base64/getEncoder) (.getBytes s)))"))
    (eval
      (read-string
      "(defn- base64-encode
         [s]
         (javax.xml.bind.DatatypeConverter/printBase64Binary (.getBytes s)))"))))

You'd then unconditionally call base64-encode from the existing patch, Erik.

I'm simultaneously horrified and rather chuffed at this ghastly hack...

Comment by Alex Miller [ 19/Oct/17 10:44 PM ]

Doing nothing is always an option.

Comment by Peter Monks [ 19/Oct/17 10:54 PM ]

Where's the fun in that?

Comment by Alex Miller [ 20/Oct/17 8:55 AM ]

As the one maintaining it down the line, the fun is in avoiding future pain.

Comment by Peter Monks [ 20/Oct/17 10:15 AM ]

How about this approach (also tested as before):

(defmacro base64-encode
  [^String s]
  (let [jvm-version (System/getProperty "java.version")]
    (if (or (clojure.string/starts-with? jvm-version "1.6")
            (clojure.string/starts-with? jvm-version "1.7"))
      `(javax.xml.bind.DatatypeConverter/printBase64Binary (.getBytes ~s))
      `(.encodeToString (java.util.Base64/getEncoder) (.getBytes ~s)))))

?

I also inverted the condition, to make the code more future proof (assuming the JVM retains java.util.Base64 going forward, of course).

Note: there's a separate (unanswered) question regarding whether this macro should be ^:private or not.

Comment by Phill Wolf [ 21/Oct/17 9:02 PM ]

What matters is the Java version that runs the program. The macro is pleasantly legible, but it tests java.version at compile-time.

Comment by Peter Monks [ 22/Oct/17 12:04 AM ]

Is this ns AOT compiled? If not, am I misunderstanding when Clojure compilation occurs?

Comment by Alex Miller [ 22/Oct/17 8:24 AM ]

This ns is AOT compiled, but this form will test version at compile-time of the user's program (not compile-time of Clojure). Usually for stuff like this we prefer to check for the presence/absence of a Class as that is less version-sensitive. Here, that might mean checking for presence of DatatypeConverter before invoking it (that also ties the decision to what you're actually doing more closely).

However, I am increasingly skeptical that any of this is worth doing.

Comment by Peter Monks [ 22/Oct/17 10:42 AM ]

Absolutely happy to have modified / alternative approaches proposed / implemented.

But going back to the requirement, I might just point out that slurp (or rather the relevant clojure.java.io fns) don’t fully support the the URI standard. And while this is clearly a deficiency of Java, given the ease of working around it in Clojure it seems to me to be worthwhile addressing (and I’m not just saying that because I have this exact use case).

Comment by Erik Assum [ 22/Oct/17 2:41 PM ]

Uploaded a new patch which unconditionally uses java.util.Base64, but inside a try/catch
This means that if the code is run on a jdk less than 8, the behaviour will be as today, whereas if run on 8 or 9,
authentication will work.

The downside to this approach is that sometime in the future when the lowest supported jdk version is 8, the try/catch code will be
redundant, and should be removed.

Comment by Peter Monks [ 25/Oct/17 12:10 PM ]

FWIW here's a workaround that appears to work, and should be a drop-in to most (all?) Clojure projects.

Thanks to Erik for showing how to "patch" IOFactory from "outside" core Clojure.

Comment by Erik Assum [ 23/Nov/17 2:37 PM ]

FWIW2 The new macro (which unfortunately is marked private), when-class could be a clean way to implement this.
https://github.com/clojure/clojure/commit/ecd15907082d31511f1ed0a249bc48fa532311f4





[CLJ-2246] spec.test/check returns the wrong value of :failure for failing tests Created: 02/Oct/17  Updated: 16/Jan/18

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

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

[org.clojure/clojure "1.9.0-beta1"]
[org.clojure/spec.alpha "0.1.123"]
[org.clojure/test.check "0.10.0-alpha2" :scope "test"]


Approval: Triaged

 Description   

When trying to manually wire clojure.test and spec for fdef'ed functions, I realized that spec.test/check returns a map with [:failure false] for failing tests. I'm (almost) sure it should return true, because spec.test/abbrev-result receives as argument the return of spec.test/check and tests the value of :failure. I couldn't produce a case where spec.test/check returned [:failure true] for failing tests. For information it returns a map without the key :failure for passing test.

Here's a simple reproduction case in the REPL :

(defn foo-fn [x] "bar")

(s/fdef foo-fn
:args (s/cat :x any?)
:ret string?)

(stest/check `foo-fn) ;; => no error, normal small map printed, no :failure entry

(defn foo-fn [x] 1)

(stest/check `foo-fn) ;; => full error map, with :failure equals false. :failure shown below

(-> (stest/check `foo-fn) first :failure) ;; => false



 Comments   
Comment by Gary Fredericks [ 02/Oct/17 7:34 PM ]

This is not a bug, though it is confusing.

stest/check is passing through (at least part of) the return value from clojure.test.check/quickcheck, which returns the value returned by the property, which is evaluated for thruthiness to determine pass vs fail. Since a non-truthy value can only be false or nil, you don't learn very much by looking at it (though it's possible that the :failure key could also have an exception under it, which would be more informative).

The next release of test.check should have a more useful and less confusing way of extracting information from a test, but I don't know for sure how that would look when using stest/check.

Comment by Khalid Jebbari [ 03/Oct/17 3:43 AM ]

I still don't understand why you don't consider it a bug. The docstring of `stest/check` says ":failure optional test failure". I expect to have a `:failure` key only when the tests fail (hence the wording optional), with some valuable info.

I'm using the `stest/abbrev-result` to display the output of `stest/check`, which expects `:failure` to be truthy to display all the valuable failure information. If `stest/check` returns `false`, there's a mismatch I consider a bug. The docstring of `stest/abbrev-result` explicitly says it receives as argument the result of check, so I'm not forcing a square peg into a round hole.

Thank you for answering me so fast and for your time.

Comment by Michael Glaesemann [ 15/Jan/18 6:23 PM ]

The false value for :failure is definitely confusing. The stest/abbrev-result is very confounding in the {:failure false} as it doesn't provide additional information that failures usual do,as Khalid Jebbari points out.

(defn abbrev-result
  "Given a check result, returns an abbreviated version
suitable for summary use."
  [x]
  (if (:failure x)
    (-> (dissoc x ::stc/ret)
        (update :spec s/describe)
        (update :failure unwrap-failure))
    (dissoc x :spec ::stc/ret)))

Here's an example showing how misleading it can be:

(require '[clojure.spec.alpha :as s]
         '[clojure.spec.test.alpha :as stest])

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

(defn adder [a b]
  (+ a b))

(s/fdef adder
        :args (s/cat :a int? :b int?)
        :ret string?)

(-> (stest/check `adder) first stest/abbrev-result)
;; => {:sym ex.check-test/adder, :failure false}

;; Writing an alternative version of `abbrev-result` that
;; checks for `true`

(defn- failure-type [x] (::s/failure (ex-data x)))
(defn- unwrap-failure [x] (if (failure-type x) (ex-data x) x))

(defn- abbrev-result [x]
  (let [failure (:failure x)]
    (if-not (or (true? failure)
                (nil? failure))
      (-> (dissoc x ::stc/ret)
          (update :spec s/describe)
          (update :failure unwrap-failure))
      (dissoc x :spec ::stc/ret))))

(-> (stest/check `adder) first abbrev-result)
;; => {:spec (fspec :args (cat :a int? :b int?) :ret string? :fn nil),
;;     :sym ex.check-test/adder,
;;     :failure false}

Again, note that any truthy :failure value is going to provide the additional details, while falsey :failure values will not.

I understand the motivation for not changing the value of the :failure key. If that value is going to be maintained, I think stest/abbrev-result should be updated to likewise test explicitly for nil and true, rather than truthy for consistency with the result of stest/check.

Comment by Khalid Jebbari [ 16/Jan/18 8:13 AM ]

Thanks a lot Michael. In the end I went with a small variation, where I don't `(dissoc x ::stc/ret)` but keep the whole check result because it includes the stack trace, the spec/explain-data map, the shrunk failing case etc.





[CLJ-2244] double division by zero inconsistency Created: 27/Sep/17  Updated: 04/Oct/17

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

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

Approval: Triaged

 Description   

Division by zero with doubles will throw an exception if both arguments are boxed, but not otherwise:

Clojure 1.9.0-beta1
user=> (/ 5.0 0.0)
##Inf
user=> (/ (identity 5.0) 0.0)
##Inf
user=> (/ 5.0 (identity 0.0))
##Inf
user=> (/ (identity 5.0) (identity 0.0))
ArithmeticException Divide by zero  clojure.lang.Numbers.divide (Numbers.java:163)


 Comments   
Comment by Erik Assum [ 28/Sep/17 1:10 PM ]

So, I guess some division by zero is somewhat undefined.
If one removes

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

then

(/ (identity 5.0) (identity 0.0))

returns infinity as the unboxed versions.

Although doing so, also causes a change to

(/ 5 0)

which then becomes `1/0`, whereas it previously threw Divide by zero.

From Numbers.java we see that unboxed math does not protect against division by zero:

static public double divide(double x, double y){
	return x / y;
}

whereas boxed versions do:

static public Number divide(Object x, Object y){
	if (isNaN(x)){
		return (Number)x;
	} else if(isNaN(y)){
		return (Number)y;
	}
	Ops yops = ops(y);
	if(yops.isZero((Number)y))
		throw new ArithmeticException("Divide by zero");
	return ops(x).combine(yops).divide((Number)x, (Number)y);
}

This inconsistency is also reproducible with clojure 1.8:

Clojure 1.8.0
Java HotSpot(TM) 64-Bit Server VM 1.8.0_144-b01
    Docs: (doc function-name-here)
          (find-doc "part-of-name-here")
  Source: (source function-name-here)
 Javadoc: (javadoc java-object-or-class-here)
    Exit: Control+D or (exit) or (quit)
 Results: Stored in vars *1, *2, *3, an exception in *e

user=> (/ 5.0 0.0)
Infinity
user=> (/ (identity 5.0) (identity 0.0))

ArithmeticException Divide by zero  clojure.lang.Numbers.divide (Numbers.java:158)
user=>
Comment by Erik Assum [ 04/Oct/17 12:43 PM ]

Any thoughts on an approach?
Both throwing on division by zero for natives and allowing it for boxed seems to be a serious change, and the tests expects an exception for boxed division by zero.





[CLJ-2240] Load user.cljc in classpath root. Created: 22/Sep/17  Updated: 22/Sep/17

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

Type: Enhancement Priority: Minor
Reporter: Sameer Rahmani Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

Right now Clojure only loads user.clj but it might be a good idea to load user.cljc if it was in classpath as well.






[CLJ-2238] NPE when print-duping Void/TYPE Created: 18/Sep/17  Updated: 18/Sep/17

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

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

Attachments: Text File 0001-CLJ-2238-handle-Void-TYPE-in-print-dup.patch    
Patch: Code
Approval: Triaged

 Description   
user=> (binding [*print-dup* true] (print Void/TYPE))

NullPointerException   java.io.PrintWriter.write (PrintWriter.java:473)
#=(identity user=>

Patch: 0001-CLJ-2238-handle-Void-TYPE-in-print-dup.patch






[CLJ-2235] Add named loop + recur-to facility for nested loops Created: 10/Sep/17  Updated: 17/Sep/17

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

Type: Feature Priority: Major
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler

Attachments: Text File 0001-CLJ-2235-implement-named-loop-recur-to.patch     Text File 0001-CLJ-2235-implement-named-loop-recur-to-separate-special-form.patch     Text File 0001-CLJ-2235-recur-to-keyword-loop-names.patch     Text File 0002-CLJ-2235-implement-recur-recur-to-as-separate-specia.patch     Text File 0002-CLJ-2235-use-keywords-as-loop-names.patch    
Approval: Triaged

 Description   

Copied from the proposal email to the Clojure/dev Google group:

https://groups.google.com/d/msg/clojure-dev/zlMGmv60MVA/beyIRTrhAgAJ

Hi,

Currently loop/recur is limited to "single-layered" loops: loop forms can occur inside other loop forms, but there is no facility for "recuring to an outer loop".

A few years ago I posted a proposal to add support for nested loops to Clojure with a proof-of-concept patch to ClojureScript with syntax and semantics that I think suffice to make nested loops feel natural while remaining a natural extension of the core loop/recur model, with the same explicit tail recursion benefits:

(loop foo [...]    ; named loop
  ...
  (loop [...]      ; intervening loop
    ...
    (loop [...]    ; innermost loop
      ...
      (recur-to foo ...)))) ; recur to the named loop;
                            ; NB. this must be in tail position
                            ; w.r.t. *all* three loops

https://groups.google.com/d/msg/clojure-dev/imtbY1uqpIc/8DWLw8Ymf4IJ
https://dev.clojure.org/display/design/Named+loops+with+recur-to
https://github.com/michalmarczyk/clojurescript/tree/recur-to
https://github.com/michalmarczyk/clojurescript/commit/feba0a078138da08d584a67e671415fc403fa093

I have now implemented a complete patch enabling the proposed feature in Clojure (the first link is to a branch based on current master, that is, the "prepare for next development iteration" commit after 1.9.0-alpha17; the second is to the current tip of this branch for future reference):

https://github.com/michalmarczyk/clojure/tree/recur-to

https://github.com/michalmarczyk/clojure/commit/212ea06d21d3b336ac35480c99170e81defaf956

I also opened a ticket in JIRA so as to have a place to post the above in the form of a patch file:

https://dev.clojure.org/jira/browse/CLJ-2235

The remainder of this email sets out the proposal in more detail, states its key properties in a somewhat rigorous form, briefly summarizes the implementation approach and discusses certain design choices made in the patch.

Overview
========

The idea is that one could write e.g.

(let [m (two-dimensional-matrix)]
  (loop iloop [i 0]           ; named loop
    (if (< i i-lim)
      (loop [j 0]             ; nested anonymous loop
        (if (< j j-lim)
          (do
            (work)
            (recur (inc j)))  ; recur to the innermost loop
          (recur-to iloop (inc i)))) ; recur to iloop
      (done))))

and, provided that each recur-to form is in tail position with respect to all its enclosing loop forms up to and including its target and the number of arguments passed to each recur-to form matches the number of loop locals of the target loop (plus one for the leading loop name argument), this should compile and behave much like nested loops in Java.

The proposed syntax is modelled on Scheme's named lets, although semantically
those are quite different - this proposal is strictly limited to expanding the loop/recur model to nested loops in a natural way. Of course named fn forms ought also to be valid recur-to targets.

Key properties of named loops and recur-to
==========================================

With the above-linked patch in place, the following rules are enforced at compilation time:

1. Each recur-to form must be in tail position with respect to all its enclosing loop forms, whether named or not, up to and including its target (which may be a named loop or fn form).

2. It is an error to specify a recur-to target which does not occur among the names of the recur-to form's enclosing loop or fn forms with respect to which it is in tail position.

3. It is not possible to recur-to across try.

4. The number of arguments passed to recur-to beyond the initial target/label argument must match the number of formal parameters of the target loop or fn form.

5. Shadowing loop names is permissible; recur-to can only target the innermost loop of the given name among those with respect to which it is in tail position. Loop locals introduced by a shadowed named loop remain visible within the shadowing loop (unless they are themselves shadowed by identically named locals).

NB. loop names are not locals. In particular, they neither shadow nor are shadowed by locals of the same name. This point merits a longer discussion; see the section on design choices at the end of this email.

The innermost loop or fn form can always be targeted using plain recur, whether it is named or not. Additionally (recur-to nil ...) is equivalent to (recur ...) (even when the innermost loop or fn form is actually named), and (loop nil [...] ...) is equivalent to (loop [...]).

Summary of the implementation approach
======================================

The patch modifies the handling of loop labels in the compiler and implements the few necessary tweaks to the loop macro.

It also introduces an optional name argument to the loop* special form. (It is optional primarily so as to avoid breaking any non-core macros that emit loop* directly.)

Finally, it renames the recur special form to recur*; recur and recur-to become macros defined in clojure.core. See the section on design choices below for alternative approaches.

Design choices
==============

1. During development, purely as a matter of convenience at that stage, I had a separate loop-as macro that accepted a name argument. I thought it reasonable to add the naming feature directly to loop, particularly since fn already takes an optional name. Still, loop-as is a valid alternative design.

2. Should it be desirable to avoid renaming the existing recur special form to recur* and reimplementing recur as a macro, a new recur-to special form could be added. (Alternatively, one could keep recur as it is while adding recur-to as a macro delegating to a new recur* special form.)

3. Should it be desirable to preserve the option of treating loop names as locals in the future, it would probably be preferable to make them shadow and be shadowed by locals now, as otherwise elevating them to the status of locals at a later point would be a breaking change. To give an example of why such a future change might be useful, if tail call elimination support arrives in a future JDK spec, one might consider whether it'd be useful to adopt a Scheme-like approach with the loop name treated as a local bound to a function with a single arglist corresponding to the loop locals of the named loop; the closure allocations this would entail could perhaps be optimized away if the local is never referenced.

It bears pointing out that if TCE support does materialize, it will enable a range of alternative designs. For example, Scheme-style named lets could then be introduced as a feature of the let macro. Thus it seems to me that it is reasonable to restrict loop/recur/recur-to to label+goto-style looping only, even in a hypothetical future with VM TCE support, and that there is no reason to afford local-like treatment to loop names; hence the current no-shadowing-interactions approach of the patch.

Cheers,
Michał



 Comments   
Comment by Alex Miller [ 11/Sep/17 1:38 PM ]

Thanks Michał, it looks like you've done a lot of good work here. I think you've just missed the window for looking at new feature stuff for 1.9 but would like to circle back in next release on this.

It seems undesirable for recur to change from special form to macro, so would probably be better to either extend the existing form or add a recur-to special form.

Did you consider other options for specifying a name, like a keyword? Keywords don't carry the expectation of lexical shadowing you have with locals so could side-step those issues maybe? Maybe they are undesirable for other reasons.

Comment by Michał Marczyk [ 17/Sep/17 8:01 PM ]

Cheers, that's good to know.

recur-to as a separate special form

That's fair enough re: changing recur. Here's a version of the patch that makes recur-to a new, separate special form. Note that it still shares the RecurExpr class in the compiler the way loop and let share LetExpr. This patch is meant to be applied on top of the previous one to make it clear what the delta is:

0002-CLJ-2235-implement-recur-recur-to-as-separate-specia.patch

https://github.com/michalmarczyk/clojure/tree/recur-to

I've also prepared a squashed version that takes the current master directly to the same state:

0001-CLJ-2235-implement-named-loop-recur-to-separate-special-form.patch

https://github.com/michalmarczyk/clojure/tree/recur-to-separate-special-form

Incidentally, in implementing this patch I had to revert a change that the original patch makes to clojure.pprint that I guess demonstrates why it's a better idea to go with a new, separate recur-to – I overlooked it when preparing the original posting, otherwise it probably would have tipped the scales for me. (The change is marked by a #_ comment that I forgot to remove in the original patch. The new squashed patch cleans it up automatically by not touching that file.)

Symbols vs keywords as loop names

I used symbols partly because fn expects the optional name argument to be a symbol if present, and so recur-to had to support symbolic names anyway (assuming here that we want to recur-to to use the same form of the recur target name that was used to introduce it, which seems reasonable); and partly just "by default", because static symbolic references generally use symbols. (clojure.core/extend uses keywords, but those aren't really static references.) Not sure if the ability to attach metadata to loop names is likely to be useful, but there's that as well.

The second part about existing usage may or may not be a major consideration, particularly since loop names are somewhat unique in that they can only be referred to by a single special form – recur-to – and are otherwise invisible in the source. This also distinguishes them from fn-introduced named recur targets which of course do double as locals.

So I suppose we could use keywords for loop names and symbols for fn names if we're happy to have metadata-less loop names. We could even allow fn forms to use keyword names if the intention is to establish a named recur target without providing a local reference to the fn instance. (That'd basically be sugar over fn + loop.)

I'll have to think some more about which approach I prefer. I still like the consistency of using symbols for recur targets everywhere (fn / loop / recur-to), but having the local/not-a-local difference be accompanied by a syntactic distinction is tempting.

In the way of some brainstorming-in-the-open, I find it interesting that by using keyword loop names now we'd keep the possibility open of adding support for symbols in the future – perhaps for those VM-TCE-based Scheme-like loop names that would provide local references to closures. Or we could make "symbol labels" a generic feature of "let-like" forms (the ones backed by LetExpr, i.e. let & loop) once TCE lands. Then we'd have consider whether it should be possible for recur-to to target such named lets… And what about plain let? Might be simpler to stick to label+goto looping in loop/recur/recur-to and Scheme-like lets supported through a separate facility (perhaps simply let), with fn something of a point of intersection of the two models (which it already is, since it does introduce recur targets even when unnamed).

As a final note, the one instance where I think the possibility of using keywords for something comparable was brought up was shorthand field access syntax – IIRC (. x :foo) / (.:foo x) was brought up as a possible syntax for what has ultimately become (. x -foo) / (.-foo x). Despite being a road not taken, I think it illustrates how one could plausibly get used to keywords/symbols in effect accessing two namespaces. (Well, in the longhand version; .:foo is technically a symbol. Using keywords for loop names would not involve affording special treatment to any class of "keyword-derived" symbols.)

In any case, I'll see about preparing a version of the patch using keywords for loop names.

Comment by Michał Marczyk [ 17/Sep/17 10:02 PM ]

Here's a first cut at a "keyword loop names" patch:

0002-CLJ-2235-use-keywords-as-loop-names.patch

This is to be applied on top of the squashed "separate special forms" patch:

0001-CLJ-2235-implement-named-loop-recur-to-separate-special-form.patch

Also attaching squashed patch for convenience:

0001-CLJ-2235-recur-to-keyword-loop-names.patch

Here's an example of the new scheme:

(let [m [[[1 2 3] [4 5 6] [7 8 9]]
                 [[10 11 12] [13 14 15] [16 17 18]]
                 [[19 20 21] [22 23 24] [25 26 27]]]]
          ((fn iloop [i ires]
             (if (== i (count m))
               ires
               (loop :jloop [j 0 jres 0]
                 (if (== j (count (get m i)))
                   (recur-to iloop (inc i) (+ ires jres))
                   (loop [k 0 kres 0]
                     (if (== k (count (get-in m [i j])))
                       (recur-to :jloop (inc j) (+ jres kres))
                       (recur (inc k) (+ kres (get-in m [i j k])))))))))
            0
            0))

Note that recur-to still uses symbols where the target is an fn form.

Also note that the approach taken in this patch has the side effect that a loop named :foo won't shadow an fn-introduced recur target named foo. If we wanted eventually to introduce non-fn-introduced recur target using symbols as names (recur-to-targetable Scheme-style let/loop forms as discussed in the previous comment), that would definitely be the way to go. If not, perhaps it's still less arbitrary than declaring that :foo shadows foo in this context?





[CLJ-2234] MultiFn.prefers() ignores the multimethod's internal hierarchy Created: 09/Sep/17  Updated: 12/Sep/17

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

Type: Defect Priority: Major
Reporter: John Alan McDonald Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: multimethods

Attachments: Text File prefersHierarchy.patch    
Patch: Code and Test
Approval: Triaged

 Description   

See https://groups.google.com/forum/#!topic/clojure/3nMn6TiBGOg, which hasn't had any response.

prefers(x,y) visits ancestors by calling parents.invoke() recursively.
This visits the parents in the global hierarchy, not the multimethod's hierarchy.
Is this the intended behavior? Why would that be?

On the assumption that prefer-method should behave the same for a local vs the global-hierarchy, below are 2 unit tests.
MultiFn-prefers-with-local-hierarchy fails with a "Multiple methods" IllegalArgumentException.
MultiFn-prefers-with-global-hierarchy succeeds.

(test/deftest MultiFn-prefers-with-local-hierarchy
  (def local-hierarchy 
    (let [h (make-hierarchy)
          h (derive h ::c0 ::b0)
          h (derive h ::d0 ::c0)
          h (derive h ::d0 ::a0)]
      h))
  (defmulti local identity :hierarchy #'local-hierarchy)
  (defmethod local ::a0 [x] [::a0 x]) 
  (defmethod local ::c0 [x] [::c0 x]) 
  (prefer-method local ::b0 ::a0)
  (test/is (= [::c0 ::d0] (local ::d0)))))

(test/deftest MultiFn-prefers-with-global-hierarchy
  (derive ::c1 ::b1)
  (derive ::d1 ::c1)
  (derive ::d1 ::a1)
  (defmulti global identity)
  (defmethod global ::a1 [x] [::a1 x]) 
  (defmethod global ::c1 [x] [::c1 x]) 
  (prefer-method global ::b1 ::a1)
  (test/is (= [::c1 ::d1] (global ::d1))))

If this is in fact wrong, the fix is pretty easy. I'll submit a patch once it's confirmed this is a real problem.



 Comments   
Comment by Alex Miller [ 11/Sep/17 1:41 PM ]

Patch welcome

Comment by John Alan McDonald [ 12/Sep/17 6:12 PM ]

I'm not sure about the change to preferMethod.
I've moved resetCache to the beginning so that
perfers() can be called with the current state of the hierarchy. Since we are in a write lock, I don't think it's necessary to call it again.





[CLJ-2225] clojure.core/assert docstring is incorrect Created: 22/Aug/17  Updated: 13/Sep/17

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

Type: Enhancement Priority: Minor
Reporter: Gordon Syme Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

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

 Description   

The assert macro has two arities, documented as (assert x) and (assert x message)

The docstring is:

Evaluates expr and throws an exception if it does not evaluate to logical true.

This is quite misleading since assert actually throws an Error. It is tempting to use assert assuming that a failure will lead to a stacktrace in logs, like any other Exception, but the reality is that the JVM will terminate.

The behaviour is correct, assert should cause a program to exit if the asserted condition is true but I regularly come across incorrect uses.

I'll work up a patch if people agree this is an issue.



 Comments   
Comment by Alex Miller [ 22/Aug/17 5:54 AM ]

Error is a subclass of Throwable, just like Exception and has no special behavior (although it does have the special intended meaning that most programs should not catch it). Whether or not your program does catch it, or exit, or log, is totally up to your program and not a property of being an Error.

The only change that I think might make sense here is to be more specific about the exception type.

Comment by Gordon Syme [ 13/Sep/17 5:46 AM ]

clj-2225-20170913.patch updates the assert macro docstring. I've wrapped the docstring at 70 characters, it seems to be a fairly common width used throughout the rest of the file.





[CLJ-2224] Support printing and reading of Java 8 java.time.Instant Created: 19/Aug/17  Updated: 20/Aug/17

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

Type: Enhancement Priority: Minor
Reporter: David Bürgin Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: instant
Environment:

Java 8


Approval: Triaged

 Description   

In Clojure 1.9 alpha, limited support for Java 8 java.time.Instant was added, namely by (conditionally) extending the Inst protocol to that type.

It would be useful to enhance support for java.time.Instant further by

  • installing a print-method and print-dup for java.time.Instant
  • providing a read-instant function for reading java.time.Instant

This functionality is already provided in Clojure 1.8 today for the types java.util.Date, java.util.Calendar, and java.sql.Timestamp; extending it to java.time.Instant would be very helpful in environments using Java 8.






[CLJ-2223] Add extra map argument to clojure.core/assert and clojure.spec/assert (like with ex-info) Created: 13/Aug/17  Updated: 14/Aug/17

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

Type: Feature Priority: Major
Reporter: Vitalie Spinu Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: assert, ex-info, spec

Attachments: Text File 0001-Add-extra-map-argument-to-clojure.spec-assert.patch     Text File 0001-Add-extra-map-argument-to-clojure.spec-assert.patch    
Approval: Triaged

 Description   

It's a common requirement to provide extra info about the context of the assertion. For instance one might want to include values of local or dynamic bindings into the assert report, or use `clojure.spec/assert` to leverage structured errors with specs outside of testing environments.

With clojure.core/assert one can splice extra data into formated message but with clojure.spec/assert there is no such feature. The message argument to clojure's assert was added in CLJ-774, and it was acknowledged there that the mechanism is not ideal. One notable benefit of passing reports as data is that editors can handle those gracefully in case of large data.

This proposal is related to CLJ-415 but has a broader scope. With an extra map one can pass any value (not just locals), and there is no danger of inadvertently flooding the REPL with large locals.



 Comments   
Comment by Alex Miller [ 13/Aug/17 12:41 PM ]

Can you start this with a use case for what you are trying to do where this would be useful? The title here is a solution but we find it is best to start with a problem, consider alternatives, and choose a solution.

Comment by Vitalie Spinu [ 13/Aug/17 1:44 PM ]

EDITED: Moved motivation into the issue's "Description".

Comment by Alex Miller [ 14/Aug/17 6:56 AM ]

Rewriting the ticket from this perspective would be a good start.

Comment by Vitalie Spinu [ 14/Aug/17 7:35 AM ]

I would be happy to rewrite, but Jira doesn't allow editing the description.

Comment by Alex Miller [ 14/Aug/17 7:58 AM ]

Sorry about that! I've given you edit rights.





[CLJ-2215] Extend-protocol for array of Object does not work on array of subtypes of Object. Created: 31/Jul/17  Updated: 31/Jul/17

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

Type: Defect Priority: Minor
Reporter: Didier A. Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: protocols

Approval: Triaged

 Description   

It appears that when you extend a protocol for "[Ljava.lang.Object;", the dispatch can not resolve for child types of "[Ljava.lang.Object;".

(defprotocol Table
  (t [this]))

(extend-protocol Table
  (Class/forName "[Ljava.lang.Object;")
  (t [this] this))

(t (make-array java.lang.String 0))
=> IllegalArgumentException No implementation of method: :t of protocol: #'test-t/Table found for class: [Ljava.lang.String;  clojure.core/-cache-protocol-fn (core_deftype.clj:568)

(t (make-array java.lang.Object 0))
=> ["[Ljava.lang.Object;" 1512480936 "[Ljava.lang.Object;@5a26a0a8"]

Yet Java is covariant on Object[]:

(instance? (Class/forName "[Ljava.lang.Object;") (make-array java.lang.String 0))
=> true
$ cat > Foo.java
public class Foo {
  public Object[] fooey;
  public Foo() {
    fooey = new String[10];
  }
}
$ javac Foo.java
$


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

Related: CLJ-1381





[CLJ-2214] Add binding conveyance to reducers. Created: 30/Jul/17  Updated: 30/Jul/17

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

Type: Enhancement Priority: Minor
Reporter: Didier A. Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reducers

Approval: Triaged

 Description   

Clojure reducers is the only parallel construct that does not support binding conveyance.

(def ^:dynamic bar "In main thread.")

(binding [bar "In reducers Thread."]
         (r/fold (fn [& args]
                   bar) (vec (range 100000))))
=> "In main thread."

You have to manually use bound-fn:

(binding [bar "In reducers Thread."]
         (r/fold (bound-fn [& args]
                           bar) (vec (range 100000))))
=> "In reducers Thread."

I propose it is enhanced to behave the same as pmap, future, agents and core.async.






[CLJ-2213] Allow multiple bindings for if-let, when-let, if-some, and when-some Created: 29/Jul/17  Updated: 04/Aug/17

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

Type: Enhancement Priority: Major
Reporter: Justin Spedding Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File clojure.core.patch     Text File clojure-core v2 8-3-2017.patch     Text File core.specs.alpha.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Description of issue:

Suppose I want to create multiple bindings with let and then execute a body. I can do that easily like so:

(let [a 1
      b (inc a)
      c (* b b)]
  [a b c])

But, if I want to do the same type of thing with if-let, I can only do so by nesting them because if-let only accepts one binding at a time.

(if-let [a 1]
  (if-let [b (inc a)]
    (if-let [c (* b b)]
      [a b c]
      "error")
    "error")
  "error")

This is very inelegant because:
1) It is not as simple to read as it would be if all of the bindings were next to each other on the same indentation
2) The else clause it duplicated multiple times.
3) The else clause is evaluated in a different context depending which binding failed. What if a was already bound to something? If the if-let shadows a, and b does not get bound, the else clause would be executed with a different value bound to a than if a was not shadowed in the first if-let. (see code below for example)

I want to be able to write this instead:

(if-let [a 1
         b (inc a)
         c (* b b)]
  [a b c]
  "error")
=> [1 2 4]

(let [a :original]
  (if-let [a :shadowed
           b false]
          a a))
=> :original

I also want to be able to do a similar thing with when-let, if-some, and when-some.

Proposed:

I re-wrote those macros to be able to handle multiple bindings. If supplied with just one binding, their behavior remains identical. If supplied with multiple bindings, they should only execute the body if every binding passed. In the case of some bindings passing and some failing in if-let or if-some, none of the bindings should leak into the else clause.

Patches:

  • clojure-core v2 8-3-2017.patch - Clojure patch with macro updates. For if-let and if-some, I had to add a bit of extra logic in order to prevent them from leaking bindings to the else clause in the case of some bindings passing and some failing. It also includes a few extra tests around each macro.
  • core.specs.alpha.patch - core.specs.alpha patch with equivalent updates to core specs


 Comments   
Comment by Alex Miller [ 29/Jul/17 4:40 PM ]

What's the relationship of this to CLJ-2007?

Comment by Justin Spedding [ 29/Jul/17 4:52 PM ]

I posted my solutions to that ticket as code in a comment. Then, you posted about the correct format of tickets and linked to the ticket creation guidelines. I figured that meant that you wanted a ticket to be made that followed the conventions.

Also, this ticket is about modifying the existing macros. CLJ-2007 was about creating 2 new macros: if-let* and when-let*.

Comment by Ghadi Shayban [ 03/Aug/17 3:30 PM ]

It is worth looking at what the JVM is intending on doing with test-and-destructure intrinsics. Brian Goetz covers this in a recent talk on pattern matching [1]

[1] https://www.youtube.com/watch?v=n3_8YcYKScw

Comment by Justin Spedding [ 03/Aug/17 9:44 PM ]

An updated patch that simplifies the generated code when 0 bindings are given to if-let and if-some





[CLJ-2212] docstring for `defmethod` is imprecise Created: 28/Jul/17  Updated: 20/Aug/17

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

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

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

 Description   

arglist:

(defmethod multifn dispatch-val & fn-tail)

docstring:

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

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

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

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



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

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

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

Adding patch

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

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





[CLJ-2211] docstring of defn is not precise about the `attr-map?` and `body` arguments Created: 28/Jul/17  Updated: 20/Aug/17

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

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

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

 Description   

The docstring for defn is:

Same as (def name (fn [params* ] exprs*)) or (def name (fn ([params* ] exprs*)+)) with any doc-string or attrs added to the var metadata. prepost-map defines a map with optional keys :pre and :post that contain collections of pre or post conditions.

The arglist is:

(defn name doc-string? attr-map? [params*] prepost-map? body)

There are two issues that made this docstring hard to understand for me:

1. The docstring does not mention attr-map? - it took me a a bit of jumping around the docs to make the leap from attr-map? to "with any [...] attrs added to the var metadata".
2. The docstring makes reference to exprs*, but the arglist refers to body.



 Comments   
Comment by Alex Miller [ 28/Jul/17 8:24 AM ]

Patch welcome, would appreciate smallest change possible.

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

I've attached a patch to address two issues:

  • change 'exprs*' to 'body' to match the arglist.
  • change 'body+)' to 'body)+' when referring to the multi-artiy form.

I didn't address "any attrs" referring to attr-map, I wasn't sure the most "Clojury" way to phrase it.

Comment by Alex Miller [ 20/Aug/17 4:59 PM ]

Seems like the body change is missing a trailing right paren?

You have: (def name (fn ([params* ] body)+)
Should be: (def name (fn ([params* ] body)+))





[CLJ-2208] Provide a means to ask a spec for its "child" specs Created: 15/Jul/17  Updated: 15/Jul/17

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

Type: Enhancement Priority: Major
Reporter: Eric Normand Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 Description   

Some kinds of operations on specs are currently hard to implement as there is no uniform way to find what "child" specs are being composed by a spec. Examples:

  • Dependency analysis
  • Deep describe (show all specs used by a top-level spec)
  • Detection of missing or invalid spec names

For example, given:

(s/def ::user-id int?)
(s/def ::user (s/keys :req [::userid])) ;; note misspelling
(s/valid? ::user {::userid "Jim"}) ;; => true but expect false

And the means to determine the "child" specs of ::user, a linter could check whether all of the keys in s/keys are specs that have been defined.

Workarounds:

1. form can be used to get the original spec form, but that must then be further interpreted (and is missing the original lexical environment in which it was created). Example attempt: https://gist.github.com/ericnormand/6cfe6809beeeea3246679e904372cca0
2. Spec form specs (CLJ-2112) are not available yet, but could be used to get a parsed representation of specs, which would still require some processing but would at least have known forms.

Proposed:

Add a mechanism to get the "child" specs a spec is composed of. Each spec implementation could then choose how to implement this in the appropriate way.



 Comments   
Comment by Eric Normand [ 15/Jul/17 8:53 AM ]

I forgot to add this proposal:

Proposal

I propose that we add a children* method to the Spec protocol. It should return a collection of specs directly referred to. The specs in the collection should be a keyword (if it is referred to by name), an instance of Spec (for nested specs), or some other value valid as a spec (such as a fn).

Comment by Alex Miller [ 15/Jul/17 8:59 AM ]

Rewrote title and some of the description to be less dependent on implementation details (which may change) and more about the problem at hand.





[CLJ-2196] Allow string keys for `s/key` specs Created: 30/Jun/17  Updated: 30/Jun/17

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

Type: Enhancement Priority: Major
Reporter: Ben Brinckerhoff Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec

Approval: Triaged

 Description   

JSON is a common data format, especially when interfacing with non-Clojure systems. All keys in JSON objects are strings (not keywords, as is common in Clojure).

It is desirable to be able to validate incoming JSON data and provide helpful error messages when data is poorly formed. Spec is an excellent tool for both, but `s/keys` only works with keyword keys.

It would be useful to be able to specify string keys, for instance, given some JSON data like

{"city" "Denver" "state" "CO"}

I would like to write a spec like:

(s/def :location/city string?)
(s/def :location/state string?)
(s/keys :req-str [:location/city :location/state])

where `:req-str` is like `:req` and `:req-un-str` would be like `:req-un`. The specs would still be fully-qualified keywords.

The current workaround:

1. Convert string keys to keyword keys using `clojure.walk/keywordize-keys`
2. Validate with spec
3. If there are problems, map over the problems and use `stringify-keys` on each val
4. Format the problems appropriately (basically, reproduce the formatting of `explain`).

This workaround is not particularly difficult, but since I suspect working with JSON is a common case, it may be useful to support this use case more directly.






[CLJ-2193] Override function spec within check Created: 28/Jun/17  Updated: 29/Jun/17

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

Type: Enhancement Priority: Major
Reporter: Michael Glaesemann Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 Description   

It's desirable to be able to override a function spec within the scope of a check.

For example:

(require '[clojure.spec.alpha :as s])
(require '[clojure.spec.test.alpha :as stest])

(defn a [x])

(s/fdef a
        :args (s/cat :x int?)
        :fn (fn [_] true))

(s/fdef b
        :args (s/cat :x int?)
        :fn (fn [_] false))

;; should pass
(stest/check `a)

(stest/instrument `a {:spec {`a `b}})
;; should fail
(stest/check `a)

;; Similar cases which should fail:

(stest/instrument `a {:spec {`a (s/fspec :args (s/cat :x int?) :fn (fn [_] false))}})
(stest/check `a)

(stest/instrument `a {:spec {`a (s/get-spec `b)}})
(stest/check `a)





[CLJ-2185] Standardize the running context of all Java to Clojure entry points. Created: 22/Jun/17  Updated: 22/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: Didier A. Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bootstrap, interop

Approval: Triaged

 Description   

Clojure always must be handed the program counter from Java, the JVM can't load right into Clojure. So there's a few ways you can do it:

1) With a gen-class
2) From an already bootstrapped REPL
3) With clojure.main
4) With the Clojure runtime (RT.java)
5) With the Clojure Java API (https://clojure.github.io/clojure/javadoc/clojure/java/api/Clojure.html)
6) With popular tools such as Leiningen and Boot

Any time you want to run some Clojure code from Java, you need to do it with one of those mechanisms, and you cannot run Clojure code without going to Java first.

The issue is that, not all mechanisms hand over execution with an equal context.

#1, #4 and #5 all hand over execution from the context that the Clojure runtime executes in. That means that you get a minimal context running inside the "clojure.core" namespace. No binding is setup, and ns points to "clojure.core".

#3 and #6 seem all to hand over execution with a similar context, but one that's different from #1, #4 and #5. That is, they set common bindings such as ns, and set the current namespace to a freshly created namespace generally called user which has clojure.core referred in it. Meaning that code executed from #3 and #6 will be able to set common bindings and won't interfere with the clojure.core namespace.

#2's context will depend on how the REPL itself was bootstrapped from Java. In most cases, clojure.main, lein, boot, it will be similar to #3 and #6.

I believe that it would be healthy for Clojure to standardize the execution context when bootstrapping Clojure from Java, so that all above mechanism behave the same. This will make it so that all code will run equally from all bootstrapping mechanism. This is not currently the case, since missing bindings and the clojure.core current namespace can cause certain rare scenarios to fail under #1, #4 and #5, but work under #3 and #6.

To not break backwards compatibility, it would be better to enhance #1, #4 and #5 so that they also set common bindings and change the current namespace to a fresh user namespace with clojure.core referred in it.

Initial discussion on the mailing list: https://groups.google.com/forum/#!topic/clojure/6CXUNuPIUyQ






[CLJ-2181] try accepts multiple catch blocks for the same class Created: 07/Jun/17  Updated: 07/Jun/17

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

Type: Defect Priority: Minor
Reporter: Justin Glenn Smith Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler
Environment:

any


Approval: Triaged

 Description   

try silently accepts multiple catch blocks for the same class, but only the first one gets called

user=> (try (/ 1 0) (catch Exception _ (println "a")) (catch Exception _ (println "b")))
a
nil





[CLJ-2180] Enhancing :path info for s/merge & s/and & s/& to indicate which subspec raised spec error Created: 07/Jun/17  Updated: 07/Jun/17

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

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

Approval: Triaged

 Description   

Description

Suppose we want to traverse a spec that caused some spec error, from the root spec embedded in the explain-data to a leaf of the pred that was the actual cause of the error.

We can usually use :path info in the explain-data for such a purpose:

user=> (s/explain-data (s/tuple integer? string?) [1 :a])
#:clojure.spec.alpha{:problems
                     ({:path [1], ;; <- indicates the 1st subspec, ie. integer?, was the cause of the error
                       :pred clojure.core/string?,
                       :val :a,
                       :via [],
                       :in [1]}),
                     :spec ...,
                     :value [1 :a]}
user=>

If we traverse the spec tree along the :path, we can eventually reach the leaf pred that raised the spec error.

In some cases, however, it doesn't hold since some specs such as s/merge, s/and and s/& don't put any clue into :path that tells which subspec actually raised the error:

user=> (s/explain-data (s/merge (s/map-of integer? string?)
                                (s/coll-of (fn [[k v]] (= (str k) v))))
                       {1 "2"})
#:clojure.spec.alpha{:problems
                     ({:path [], ;; <- doesn't tell us anything at all
                       :pred (fn [[k v]] (= (str k) v)), ;; <- we don't know which subspec this pred occurs in
                       :val [1 "2"],
                       :via [],
                       :in [0]}),
                     :spec ...,
                     :value {1 "2"}}
user=>

To achieve our purpose even in those cases, we have to make a nondeterministic choice: that is, choose a subspec arbitrarily and try traversing it down, and if something is wrong along the way, then backtrack to another subspec and so on.

From my experience that I implemented that backtracking algorithm in a library I'm working on (repo), I think it's much harder to implement correctly than necessary. In fact, my implementation is probably broken in some corner cases, and I don't even know if it's possible in theory to implement it completely correctly.

Proposal

To make it easier to implement the spec traversal, this ticket proposes adding the index into :path that indicates which subspec raised the spec error for s/merge, s/and and s/&, as follows:

user=> (s/explain-data (s/merge (s/map-of integer? string?)
                                (s/coll-of (fn [[k v]] (= (str k) v))))
                       {1 "2"})
#:clojure.spec.alpha{:problems
                     ({:path [1], ;; <- indicates the 1st subspec, ie. (s/coll-of (fn [[k v]] (= (str k) v))) has the actual cause of the error in it
                       :pred (fn [[k v]] (= (str k) v)),
                       :val [1 "2"],
                       :via [],
                       :in [0]}),
                     :spec ...,
                     :value {1 "2"}}
user=>

The enhancement, though it is indeed a breaking change, should reduce radically the effort needed to write the code traversing specs along the :path.






[CLJ-2174] Spec generated exceptions/error messages are a regression in terms of the out-of-the-box experience with Clojure. Created: 01/Jun/17  Updated: 10/Nov/17

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

Type: Defect Priority: Major
Reporter: Russ Olsen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, spec
Environment:

All


Approval: Triaged

 Description   

While it is clear that spec has a lot of advantages in terms of a uniform way to specify the shape of behavior, using spec to catch programming and API errors within Clojure itself has led to error messages that more verbose and less clear than what was there in previous versions. For example if I supply a bad name to defn, in version an earlier version of Clojure I got a clear, relatively English language messages back:

Clojure 1.7.0

user=> (defn 44 [x] x)
IllegalArgumentException First argument to defn must be a symbol clojure.core/defn--4156 (core.clj:281)

In the current 1.9 release, the same mistake generates the following:

Clojure 1.9.0-master-SNAPSHOT

user=> (defn 44 [x] x)
CompilerException clojure.lang.ExceptionInfo: Call to clojure.core/defn did not conform to spec:
In: [0] val: 44 fails spec: :clojure.core.specs.alpha/defn-args at: [:args :name] predicate: simple-symbol?
:clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x3c0f93f1 "clojure.spec.alpha$regex_spec_impl$reify__1200@3c0f93f1"]
:clojure.spec.alpha/value (44 [x] x)
:clojure.spec.alpha/args (44 [x] x)
#:clojure.spec.alpha{:problems [{:path [:args :name], :pred clojure.core/simple-symbol?, :val 44, :via [:clojure.core.specs.alpha/defn-args :clojure.core.specs.alpha/defn-
args], :in [0]}], :spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x3c0f93f1 "clojure.spec.alpha$regex_spec_impl$reify__1200@3c0f93f1"], :value (44 [x] x), :args
(44 [x] x)}, compiling:(NO_SOURCE_PATH:1:1)

There is a similar situation with let. Here is the behavior in earlier versions:

user-> (let (a 1) (println a))
IllegalArgumentException let requires a vector for its binding in user:1 clojure.core/let (core.clj:4309)

And in 1.9:

user=> (let (a 1) (println a))

CompilerException clojure.lang.ExceptionInfo: Call to clojure.core/let did not conform to spec:
In: [0] val: (a 1) fails spec: :clojure.core.specs.alpha/bindings at: [:args :bindings] predicate: vector?
:clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x31dc339b "clojure.spec.alpha$regex_spec_impl$reify__1200@31dc339b"]
:clojure.spec.alpha/value ((a 1) (println a))
:clojure.spec.alpha/args ((a 1) (println a))
#:clojure.spec.alpha{:problems [{:path [:args :bindings], :pred clojure.core/vector?, :val (a 1), :via [:clojure.core.specs.alpha/bindings
:clojure.core.specs.alpha/bindings], :in [0]}], :spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x31dc339b
"clojure.spec.alpha$regex_spec_impl$reify__1200@31dc339b"], :value ((a 1) (println a)), :args ((a 1) (println a))}, compiling:(NO_SOURCE_PATH:1:1)

Yes all of the information – and more – is there in the 1.9 version.
But the spec error messages are likely to be incomprehensible to anyone relatively new to Clojure and adds to the cognitive load of even experienced Clojure programmers.



 Comments   
Comment by Russ Olsen [ 01/Jun/17 9:15 AM ]

Typos in that first sentence, should have read 'shape of DATA' not behavior, and 'error messages that ARE more verbose'.

Comment by Alex Miller [ 01/Jun/17 2:19 PM ]

Hey Russ, part of this is actually a bug that crept into alpha17 that is tracked with a patch here: https://dev.clojure.org/jira/browse/CLJ-2171

But also, I would like to overhaul the instrument exception reporting as I think it could be a lot clearer about the signature being invoked and how it is wrong.





[CLJ-2173] LispReader.java and EdnReader.java exception messages could be much more informative. Created: 31/May/17  Updated: 31/May/17

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

Type: Enhancement Priority: Major
Reporter: Russ Olsen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, reader
Environment:

Any


Approval: Triaged

 Description   

The messages in the exceptions thrown by the readers would be much more informative if they included readily available information. There are many instances of this, but to name a few specific instances (all from LispReader.java, though there in most cases there are corresponding problems in EdnReader.java):

  • If the RegexReader class hits an unexpected EOF, it reports "EOF while reading regex". It would be helpful if the message included the first few characters of the regex it was trying to read – available in sb – as a guide to the person trying to locate the problem.
  • The same logic applies to StringReader.
  • In NamespaceMapReader, the error thrown if the namespaced map is not in fact a map could include the namespace symbol.
  • Whenever an odd number of elements in a map is detected, the exception could at least report the number of elements that the bad map did include, something like: "Map literal cannot contain 7 forms. Map literals must contain an even number of forms." Even better would be the first few forms.
  • The "Metadata can only be applied to IMetas" exception in MetaReader is not nearly as helpful as it could be. At the very least it should report the class of the thing that is not an IMeta.
  • With an additional argument, readDelimitedList could report the kind of thing that it was reading in the event that it hit the EOF. Without the additional argument it still report that it hit an EOF while trying to read the first or 4th or 29 element of a collection.





[CLJ-2172] Error message for non integer index into vector could be improved. Created: 31/May/17  Updated: 31/May/17

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

Type: Enhancement Priority: Minor
Reporter: Russ Olsen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

Approval: Triaged

 Description   

The exception generated in APersistentVector/invoke and also in APersistentVector/assoc when the index is not an integer would be better if it included the class of the object actually passed in and would be better still if it included the actual value.



 Comments   
Comment by Alex Miller [ 31/May/17 7:25 PM ]

Needs assessment of current paths through core that reach this (and whether the change in question would be better or worse when that happens).





[CLJ-2169] conj has out-of-date :arglists Created: 27/May/17  Updated: 30/Nov/17

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

Type: Defect Priority: Trivial
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File 0001-CLJ-2169-add-nullary-unary-overloads-of-conj-to-its-.patch     Text File 0002-CLJ-2169-add-nullary-unary-overloads-of-conj-to-its-.patch    
Patch: Code
Approval: Triaged

 Description   

conj has had nullary and unary overloads since 1.7.0, but its :arglists still only list [coll x] and [coll x & xs].

Prescreened by: Alex Miller



 Comments   
Comment by Michał Marczyk [ 27/May/17 6:05 PM ]

It occurs to me that perhaps the docstring could be updated too to explain (conj).

The new 0002-… patch includes the :arglists change of the 0001-… patch and adds the sentence "(conj) returns []." to the docstring immediately after "(conj nil item) returns (item).".





[CLJ-2164] case fails when a single single clause with an empty test seq is used Created: 22/May/17  Updated: 22/May/17

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

Type: Defect Priority: Minor
Reporter: Chris Blom Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001.patch    
Patch: Code and Test
Approval: Triaged

 Description   

It is not possible to use case with a single empty seq of options, or with a single seq of options and a default clause.

I would expect

(case 1 () :a :none)

to return :none, instead it fails with an uninformative exception: "Unhandled clojure.lang.ArityException: Wrong number of args (-2) passed to: core/max"

I would expect (case 1 () :a) to fail with "java.lang.IllegalArgumentException: No matching clause", but instead it also fails with
"Unhandled clojure.lang.ArityException: Wrong number of args (-2) passed to: core/max"

This seems inconsistent, as passing an empty list of options is fine when there are other alternatives:

(case 1 () :a 2 :b :none)

returns :none, as expected

The attached patch removes the test-clause pairs with empty test lists before further conversion to case*, and adds tests.



 Comments   
Comment by Chris Blom [ 22/May/17 8:54 AM ]

oops, typo in the title (duplicated "single")

Comment by Alex Miller [ 22/May/17 8:55 AM ]

An empty match list in case seems like it should be an error - maybe this should fail to compile rather than ignoring?

Comment by Chris Blom [ 22/May/17 9:07 AM ]

An empty list of options is currently supported when multiple clauses are given, so failing to compile on empty lists would be a breaking change.

This works in 1.8:

(case 1
() :never-happens
1 :ok
:default)
=> :ok

But this does not:

(case 1
() :never-happens
:default)
=> throws clojure.lang.ArityException: Wrong number of args (-2) passed to: core/max

Only failing when no other clauses are given seems very inconsistent to me.

Comment by Nicola Mometto [ 22/May/17 9:39 AM ]

an empty list doesn't make any sense in case as the correct way to match a literal empty list is `(case () (()) :empty)`. I don't see any value in making it not throw and my vote is to have the `case` macro complain at compile time every time a `()` ise used

Comment by Alex Miller [ 22/May/17 9:57 AM ]

() would never match anything now, so failing on () would not break any existing case that matches.

The one case I can imagine might exist though is a macro that creates a case and could potentially programmatically create an empty case list? Something like this:

(defmacro make-case [xs] `(defn ~'foo [e#] (case e# ~xs "matched" "nope")))
Comment by Chris Blom [ 22/May/17 10:00 AM ]

I'm not using this to match empty lists, I ran into this corner case when generating case statements from a DSL.
While it is a pathological case, I disagree that is does not make any sense, an empty list here simply represents no alternatives,
so the clause wil never match and its result-expr will never run.

My point is that now (in clojure 1.8) this is allowed:

(case a
() :never-happens
1 :a
2 :b
:default)

it is equivalent to (as an empty list never matches)

(case a
1 :a
2 :b
:default)

But

(case a
() :never-happens
:default)

gives an uninformative error.

I argue that it should be equivalent to

(case a
:default)

as rejecting empty lists in case statements in general would be a breaking change,
and only rejecting empty lists when no other clauses are present is inconsistent.

Comment by Chris Blom [ 22/May/17 10:11 AM ]

() would never match anything now, so failing on () would not break any existing case that matches.

As () in case is allowed in clojure =<1.8 (just not when its the only clause), letting the compiler reject it would potentially break existing code.

The one case I can imagine might exist though is a macro that creates a case and could potentially programmatically create an empty case list?

That is exactly how i ran into this problem





[CLJ-2160] LispReader allows no-ops macros to sneak in certain other forms (namespaced maps, tagged literals and anonymous arguments) Created: 25/Apr/17  Updated: 25/Apr/17

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

Type: Defect Priority: Minor
Reporter: Christophe Grand Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader

Attachments: Text File clj2160-2.patch     Text File clj2160.patch    
Patch: Code
Approval: Triaged

 Description   

No-op macros are line comments (starting with #! or ;), #_, reader conditional (splicing or not) with no matching feature.
Furthermore once a no-op macro has been read regular whitespace are allowed anew.

Examples:
Namespaced map #foo{:bar :baz}

#:#_()#! bang bang
#?(:whatever 42); now a blank line

#?@(:default ())foo
{:bar :baz}

Tagged literal #inst "2017-04-24T09:11:29.878-00:00"

##_()#! bang bang
#?(:whatever 42); now a blank line

inst "2017-04-24T09:11:29.878-00:00"

Anonymous argument: #(do %1)

#(do %#_()#! bang bang
#?(:whatever 42); now a blank line

#?@(:default ())1)

In addition anonymous arguments implementation is leaky (any %n is accepted as long as n is (-2.0 -1.0] (mapping to %&) and [1.0 Infinity) and any representation can be used (bigdec or bigint or float or integers in any basis).

#(list %#_(first arg)1.00000001 %#_(secong arg)2r10 %#_(rest arg)-1.5)


 Comments   
Comment by Christophe Grand [ 25/Apr/17 6:31 AM ]

The patch extracts the body from the read loop to expose a readSome method that returns either a form or the reader (if no valued form has been read starting at the current position).
This patch also adds a regex pattern to validate anonymous args.

Comment by Christophe Grand [ 25/Apr/17 7:35 AM ]

clj2160-2 is clj2160 + two redundant checks that were not removed





[CLJ-2158] multi-spec retag generator in conflict with user tag spec/gen Created: 22/Apr/17  Updated: 25/Apr/17

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

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

JVM


Approval: Triaged

 Description   

Problem: multi-spec does generate retag values on its own from values for which methods have been implemented. However a user can have purposefully speced the retag key differently and multi-spec will generate incompatible values (resulting in a such-that failure). An example is hierarchy dispatch where methods dispatch values aren't necessarily the valid tag values.

Proposed solution: When generating the retag value, multi-spec should first try the existing spec for that key and generate "such-that" it is a possible dispatch value for the multimethod, only generate direclty from the multimethod based mechanism iff there is no spec for the tag key.



 Comments   
Comment by Leon Grapenthin [ 25/Apr/17 3:00 AM ]

Improved proposed solution to cover both "user spec is a subset of dispatch values" and vice versa.





[CLJ-2155] clojure.string/index-of has some ^long type hints on let bindings that don't actually do anything Created: 19/Apr/17  Updated: 19/Apr/17

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

Type: Defect Priority: Trivial
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Approval: Triaged

 Description   

^long type hints on let binding values don't do anything:

user=> (def x 1)
#'user/x
user=> (set! *warn-on-reflection* true)
true
user=> (let [w ^long x] (Long/valueOf w))
Reflection warning, NO_SOURCE_PATH:13:18 - call to static method valueOf on java.lang.Long can't be resolved (argument types: unknown).
1
user=> (let [w (long x)] (Long/valueOf w))
1
user=>

but clojure.string/index-of has at least two cases of them, and even if they did do something, there is no reflective code that would take advantage of those type hints.






[CLJ-2154] Spec macros keys and keys* silently ignores non-keywords given in the vectors for named arguments :req and :req-un Created: 17/Apr/17  Updated: 17/Apr/17

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

Type: Defect Priority: Minor
Reporter: Rovanion Luckey Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

Ubuntu 16.04 32-bit, Clojure 1.9.0-alpha15


Approval: Triaged

 Description   

If we try to pass a non-keyword to `clojure.spec/keys` or `clojure.spec/keys*` on the named argument `:opt` or `:opt-un` we get an assertion error:

(spec/valid? (spec/keys :opt ['a 5]) {})
;1. Caused by java.lang.AssertionError
;   Assert failed: all keys must be namespace-qualified keywords
;   (every? (fn* [p1__13667#] (c/and (keyword? p1__13667#) (namespace
;   p1__13667#))) (concat req-keys req-un-specs opt opt-un))

(spec/valid? (spec/keys* :opt-un ['a 5]) {})
;1. Caused by java.lang.AssertionError
;   Assert failed: all keys must be namespace-qualified keywords                    
;   (every? (fn* [p1__13667#] (c/and (keyword? p1__13667#) (namespace               
;   p1__13667#))) (concat req-keys req-un-specs opt opt-un))

But if we do the same for the named arguments `:req` and `:req-un` the arguments are silently ignored and the call to `keys` returns a spec matching any map without any requirements:

(spec/valid? (spec/keys :req ['a 5]) {})
=> true
(spec/valid? (spec/keys :req-un ['a 5]) {})
=> true
(spec/valid? (spec/keys* :req ['a 5]) {})
=> true
(spec/valid? (spec/keys* :req-un ['a 5]) {})
=> true

An assertion should probably be thrown for the `:req(-un)?` args too.






[CLJ-2145] locals closed over by a ^:once fn aren't cleared if the fn is in a branch Created: 08/Apr/17  Updated: 11/Apr/17

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler, locals-clearing

Attachments: Text File 0001-CLJ-2145-fix-clearing-of-locals-closed-over-by-a-FNO.patch    
Patch: Code
Approval: Triaged

 Description   

Minimal case:

(fn foo [x]
  (if true
    (^:once fn* []
     ;; x is not cleared here
     x)))

This is a severe bug as it means that every local used inside a loop or try/catch expression that the clojure compiler internally hoists in a FNONCE, in a conditional branch, cannot be cleared at the moment.

As a concrete example reported in slack,

;; THIS OOMs
(defn test1 [x]
  (if true
    (do
      (try (doseq [_ x] _))
      1)
    0))

(test1 (take 1000000 (range)))

;; THIS DOESN'T OOM 
(defn test2 [x]
  (do
    (try (doseq [_ x] _))
    1))

(test2 (take 1000000 (range)))

Approach: don't set a new clearing frame if the fn is ^:once and there's an existing clearing frame
Patch: 0001-CLJ-2145-fix-clearing-of-locals-closed-over-by-a-FNO.patch






[CLJ-2135] clojure.spec/Spec implementations that don't implement IObj get silently dropped in s/def Created: 23/Mar/17  Updated: 23/Mar/17

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

Type: Enhancement Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

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

 Description   
(deftype MySpec []
  s/Spec
  (conform* [_ x]
    ::s/invalid))

(s/def ::x (MySpec.))

(s/explain ::x :foo)

This will fail with a "Unable to resolve spec: :user/x" exception, but the def succeeded. Switching the deftype to defrecord fixes the problem.

Cause: The with-name function has cond options for ident?, regex?, and IObj. If none of these succeed, there is no fallthrough case and the s/def will silently return nil.

Proposed: Throw an error in the fallthrough case.

Patch: clj-2135.patch






[CLJ-2134] Update the docstring of `with-redefs` to reflect how little the macro should be used Created: 23/Mar/17  Updated: 23/Mar/17

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

Type: Enhancement Priority: Minor
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Approval: Triaged

 Description   

Currently the docstring for `with-redefs` recommends itself for use in testing. However there are a number of reasons why using this macro for testing is suboptimal:

  • `with-redefs` "bindings" are not transferred to new threads since it's a global mutation
  • users can get runtime errors if they redef a primitive type-hinted function to a function taking only objects
  • If parts of the body of `with-redefs` is delayed (via a delay, go block, etc.) that code may not see the new root
  • The mutation is global so it "leaks" outside the current scope into other code that may currently be running in another thread
  • Clojure tends to shun global mutation, and yet this macro isn't marked with a `!` nor properly warns users about the dangers mentioned here

Due to these reasons I often encounter new users using `with-redefs` without understanding the ramifications of doing so. All this behavior makes sense if a user understands how Vars work, but that's a lot of knowledge to take on for a new user.

Suggestion:
Remove the suggestion that `with-redefs` be used in testing
Add a few notes of warning about global mutation, and concurrency issues with the macro.






[CLJ-2129] Enhance CompilerException to optionally return the invalid form Created: 16/Mar/17  Updated: 15/May/17

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

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

Approval: Triaged

 Description   

Currently CompilerException wraps errors that occur at compile time and adds file/line/col info. Some tools could do more with the failing form, particularly if it includes useful meta.

Proposal is to add a CompilerException constructor that also takes the form (Object) and conveys it. Existing uses like CLI and REPL would do nothing different, but a tooling user of the Compiler could use that information.

Possible issue: if the form is lazy or large?



 Comments   
Comment by Thomas Heller [ 16/Mar/17 12:29 PM ]

I added the latest form to the CompilerException when available but that form contains basically no metadata.

user=> (defn x
  :foo)
CompilerException clojure.lang.ExceptionInfo: Call to clojure.core/defn did not conform to spec:
...

user=> (binding [*print-meta* true] (prn (.-form *e)))
^{:line 17, :column 1} (defn x :foo)

That information is already present in the CompilerException. Having the form provides minimal benefit in this case since it has lost all other source information and we cannot tell how this look in the the source. To get a source mapping for tools so they can highlight the correct area it would still need to read the form itself (and probably not using the form from the Exception).

Comment by Thomas Heller [ 16/Mar/17 12:37 PM ]

Looking into LINE_BEFORE, COLUMN_BEFORE, LINE_AFTER, COLUMN_AFTER now to potentially provide the exact boundaries of the form if possible.

Comment by Thomas Heller [ 16/Mar/17 2:00 PM ]

I added the current reader location to the Compiler Exception [1].

user=> (load-file "/Users/zilence/code/shadow-devtools/src/dev/demo/defn_error.clj")
CompilerException clojure.lang.ExceptionInfo: Call to clojure.core/defn did not conform to spec:
...
user=> (.. *e -location -lineBefore)
4
user=> (.. *e -location -lineAfter)
6
;; column is 1 before/after, source is
(defn x
  :foo)

This would at least allow extracting the entire source string of the form easily. When using a reader however it could start at the location already provided by the CompilerException and it would find the end on its own. The bindings required for the reader location are also lost when working in a REPL, so it always points to 0/0-0/0. Not sure this is worth pursuing.

[1] https://github.com/clojure/clojure/compare/master...thheller:CLJ-2128





[CLJ-2126] Can set! to fields of a defrecord Created: 14/Mar/17  Updated: 14/Mar/17

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

Type: Defect Priority: Major
Reporter: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Attachments: Text File 0001-CLJ-2126-don-t-assign-final-fields.patch    
Patch: Code
Approval: Triaged

 Description   

It is possible to set! to fields of a defrecord even though they are final.

(defprotocol SetA (seta [x a]))
=> SetA
(defrecord X [a]
  SetA
  (seta [this newa]
    ; Next line should error at compile time, does not.
    ; (However (set! a newa) does error correctly.)
    (set! (.a this) newa)))
=> user.X
(def x (->X 0))
=> #'user/x
x
=> #user.X{:a 0}
(seta x 1) ;; This should not run.
=> 1
x
=> #user.X{:a 1}

There are two issues here:

  1. The Clojure compiler does not detect that (set! (.a this) x) is assignment to a final field. This could be enhanced. Nicola Mometto has discovered why and believes he has a straightforward patch.
  2. The JVM bytecode verifier only performs the necessary final-assignment check on classfiles version 9 and above: https://bugs.openjdk.java.net/browse/JDK-8159215 (Clojure generates version 6 classfiles.) This is out of our hands.

Approach: make the compiler fail at compile time if trying to set! a field that's final

Patch: 0001-CLJ-2126-don-t-assign-final-fields.patch






[CLJ-2124] Catch multiple exceptions in a single catch block Created: 12/Mar/17  Updated: 15/May/17

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

Type: Feature Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: exceptions, try-catch

Approval: Triaged

 Description   

Java 7 and up support multi-catch exceptions (http://www.oracle.com/technetwork/articles/java/java7exceptions-486908.html). It would be handy if Clojure also supported them to prevent catching something like Exception and then writing manual logic to check the Exception type, or duplicating the logic over multiple catch blocks.

A possible syntax for this could be:

(try (fn-that-throws)
     (catch (UnknownHostException NoRouteToHostException) e
       (go-offline)))

Prior art for this is a try* macro: https://gist.github.com/Gonzih/5814945.

One nuance to handle is

Edit: Note that in Java 7, you cannot both catch ExceptionA& ExceptionB in the same time, if ExceptionB is inherited(directly or indirectly) from ExceptionA. Compiler will complain: The exception ExceptionB is already caught by the alternative ExceptionA. - http://stackoverflow.com/a/3495968/826486

I tried searching to see if this had been asked already, but got mountains of results. I didn't see anything in the first few pages though.






[CLJ-2109] Protocol methods not instrumented Created: 13/Feb/17  Updated: 13/Feb/17

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

Type: Defect Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: spec
Environment:

Clojure 1.9.0-alpha14


Approval: Triaged

 Description   

Spec instrument does not work on protocol methods. Invalid arguments will be accepted silently with no error. Protocol vars are included in the return value of (instrument).

Steps to reproduce

(require
 '[clojure.spec :as s]
 '[clojure.spec.test :as test])

(defprotocol P
  (method [this arg]))

(defrecord R []
  P
  (method [this arg]
    (str "R.method called with " (pr-str arg))))

(s/fdef method
  :args (s/cat :this any?
               :arg number?))

(defn wrapped [this arg]
  (method this arg))

(s/fdef wrapped
  :args (s/cat :this any?
               :arg number?))

(test/instrument)

(println (method (->R) "not a number"))

(println (wrapped (->R) "not a number"))

This code produces the output:

R.method called with "not a number"
clojure.lang.ExceptionInfo: Call to #'user/wrapped did not conform to spec:
In: [1] val: "not a number" fails at: [:args :arg] predicate: number?
...

Possible resolutions

1. Add support to instrument for protocol methods
2. Document that instrument does not work on protocol methods, do not return protocol method Vars from (instrument), throw exception if protocol method Vars are included in the symbols passed to (instrument syms)

See also

CLJ-1941 describes a different case where instrument does not work. This issue was identified in a comment.

Workarounds

This issue can be avoided by wrapping protocol methods in normal functions and spec'ing the functions. This is already common practice.






[CLJ-2092] deftype instances with mutable fields cannot be compiled Created: 24/Dec/16  Updated: 31/Dec/16

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

Type: Defect Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, deftype

Approval: Triaged

 Description   

When evaluating or compiling an implementer of clojure.lang.IType, the compiler tries to reflectively access its fields. This fails, when a field is marked mutable (hence private):

Clojure 1.9.0-master-SNAPSHOT
user=> (deftype T [^:unsynchronized-mutable t])
user.T
user=> (T. :t)
#object[user.T 0x2654635 "user.T@2654635"]
user=> (eval (T. :t))
CompilerException java.lang.IllegalArgumentException: No matching field found: t for class user.T
            Reflector.java:  271  clojure.lang.Reflector/getInstanceField
             Compiler.java: 4724  clojure.lang.Compiler$ObjExpr/emitValue
             Compiler.java: 4851  clojure.lang.Compiler$ObjExpr/emitConstants
             Compiler.java: 4529  clojure.lang.Compiler$ObjExpr/compile
             Compiler.java: 4049  clojure.lang.Compiler$FnExpr/parse
             Compiler.java: 6866  clojure.lang.Compiler/analyzeSeq
             Compiler.java: 6669  clojure.lang.Compiler/analyze
             Compiler.java: 6924  clojure.lang.Compiler/eval
             Compiler.java: 6890  clojure.lang.Compiler/eval
                  core.clj: 3105  clojure.core/eval
...

For classes that don't implement IType, no such problem exists.

user> (deftype* user/U user.U
        [^:unsynchronized-mutable u]
        :implements [])
nil
user> (eval (user.U. :u))
#object[user.U 0x34699051 "user.U@34699051"]

This problem commonly occurs, when implementing a tagged literal for a deftype with cached hash.



 Comments   
Comment by Alex Miller [ 31/Dec/16 12:01 PM ]

Yeah, this is interesting. The compiler compiles a deftype into a call to the constructor with the current values of the fields, but mutable fields are not accessible. One alternative would be to provide some standard method to "read" the field set rather than relying on reflection. (Another would be changing the access modifiers for mutable fields but I think that's probably a non-starter.)





[CLJ-2090] Improve clojure.core/distinct perf by using transient set Created: 23/Dec/16  Updated: 04/Jan/17

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

Type: Enhancement Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance, transducers, transient

Attachments: Text File clj-2090-use-transient-set-in-distinct-2.patch    
Patch: Code
Approval: Triaged

 Description   

Current implementation of clojure.core/distinct uses persistent set. This patch improves performance of lazy arity by ~25%-30% and transducer by ~40%-50% by using transient set instead.

10 elements
(doall (distinct coll)) 	 5.773439 µs => 4.179092 µs (-27%)
(into [] (distinct) coll) 	 3.238236 µs => 1.943254 µs (-39%)

100 elements
(doall (distinct coll)) 	 67.725764 µs => 42.129993 µs (-37%)
(into [] (distinct) coll) 	 35.702741 µs => 16.495947 µs (-53%)

1000 elements
(doall (distinct coll)) 	 540.652739 µs => 399.053873 µs (-26%)
(into [] (distinct) coll) 	 301.423077 µs => 164.025500 µs (-45%)

10000 elements
(doall (distinct coll)) 	 3.439137 ms => 3.058872 ms (-11%)
(into [] (distinct) coll) 	 1.437390 ms => 848.277178 µs (-40%)

Benchmarking code: https://gist.github.com/tonsky/97dfe1f9c48eccafc983a49c7042fb21



 Comments   
Comment by Alex Miller [ 23/Dec/16 8:52 AM ]

You can't remove the volatile - you still need that for safe publication in multi threaded transducing contexts.

Comment by Nikita Prokopov [ 23/Dec/16 11:50 AM ]

Alex Miller How do you mean?

  • I don’t update seen link because transient set can be mutated in-place
  • Are transducers meant to be used from multiple threads? Because even existing implementation clearly has race condition. I imagine fixing that would be costly (we’ll need a synchronized section), so maybe it should be a specialized transducer that you use only when needed?
Comment by Alex Miller [ 23/Dec/16 12:26 PM ]

Transient sets can NOT be mutated in place - you must use the return value.

Yes, transducers are used from multiple threads in (for example) transducer chans in core.async go blocks.

Comment by Alex Miller [ 23/Dec/16 12:28 PM ]

I should also say transducers are not expected to be used from more than one thread at a time, so there are no race problems. But being used from multiple threads over time requires proper safe publication.

Comment by Nikita Prokopov [ 24/Dec/16 3:07 AM ]

But being used from multiple threads over time requires proper safe publication.

Does that imply that no transients could be used in transducers (because underlying arrays on which transient impl is based are mutated in place, so different threads could potentially see different states of transient object)?

Does that also mean that partition-by and partition-all should be fixed (they use java.util.ArrayList which, being array of references, has no safe publication semantics)?

Transient sets can NOT be mutated in place - you must use the return value.

I was thinking that clojure/core.clj and clojure.lang.ATransientSet.java are both part of Clojure internals, colocated, so can share a little bit of internal knowledge about each other. It seems safe to do that, because that knowledge does not leak outside, and, if at any point impl of ATransientSet would change, core.clj could be updated accordingly in the same release. I wouldn’t do that in any third-party library, of course.

Comment by Alex Miller [ 24/Dec/16 9:13 AM ]

Does that imply that no transients could be used in transducers (because underlying arrays on which transient impl is based are mutated in place, so different threads could potentially see different states of transient object)?

Transients require only that they are asked by no more than a single thread at a time and so are safe to use in a transducer. However, they should guarantee safe publication. core.async channels already do this as an artifact of their implementation, but other transducing contexts may not.

Transients should NEVER be used as "mutate in place", regardless of concurrency. While they will appear to "work" in some circumstances, this is never correct (eventually an update operation will return a new instance and if you are mutating in place, your data will then be missing). This is discussed and correct examples are shown at http://clojure.org/reference/transients.

Does that also mean that partition-by and partition-all should be fixed (they use java.util.ArrayList which, being array of references, has no safe publication semantics)?

That's something Rich and I are discussing but, probably.

Comment by Nikita Prokopov [ 24/Dec/16 12:56 PM ]

Alex Miller Here’s quick test that shows that changes to transient set (which is nothing more but a wrapper around transient map) made in one thread are not always visible from another thread.

https://gist.github.com/tonsky/62a7ec6d539fc013186bee2df0812cf6

That means that if we try to use transients for e.g. distinct it will miss duplicate items

Comment by Nikita Prokopov [ 24/Dec/16 1:02 PM ]

Removed transients from transducer arity of distincts because transducers might be accessed from multiple threads

Comment by Nikita Prokopov [ 24/Dec/16 1:12 PM ]

Maybe that doc http://clojure.org/reference/transients should be updated re: transients are not safe to use from multiple threads because changes made by one thread are not necessarily visible to another. Even if they don’t compete

Comment by Alex Miller [ 31/Dec/16 12:54 PM ]

I would say that test is demonstrating a bug in transient sets/maps and you should file a ticket for that as it's a lot more important than this enhancement.

distinct should be able to use transients in both the transducer and lazy seq impls. The issue with contains? not working on transients is actually a separate ticket - http://dev.clojure.org/jira/browse/CLJ-700 that will likely require some class hierarchy rearrangement. I don't think we would take this change until that is fixed (so that you can avoid relying on the class and Java method variants).

Comment by Nikita Prokopov [ 04/Jan/17 11:47 AM ]

I have to admit my test was demonstrating something else: there were no proper thread isolation. So it was a concurrency issue, not “safe publication” issue. My current understanding is this:

Transients require thread isolation. Use of a particular transient instance should be controlled either by using it in an single-threaded scope, or in a framework that enforces this.

That guarantee implicitly presumes that there’s happens-before relation between transient usage from multiple threads. There’s no other way to define “only one thread is in this section at a time”.

That, in turn, means that all writes that happened in thread 1 are visible in thread 2, regardless to volatility of the variables involved. In fact, we can remove all volatiles from transients implementation and probably make them faster, because, by asking “no more than one thread at a time” we enforce users to establish happens-before between sections, and that would give us all the safe publication guarantees we need.

Is my understanding correct? Am I missing something?

Comment by Nikita Prokopov [ 04/Jan/17 11:55 AM ]

Also, long-living transients (e.g. in a transducers associated with a queue, for example) will hold a reference to a thread that created them. Is that a bad thing? Should we switch to boolean flag instead?





[CLJ-2081] for macro spec should know :let can't go in the first position Created: 09/Dec/16  Updated: 09/Dec/16

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

Type: Enhancement Priority: Minor
Reporter: lvh Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 Description   

The for macro does not support :let in the first position. This was reported in CLJ-207 and a patch was produced but rejected. Since we have spec for macros now, it might be an opportunity to provide a better error message. (I think first-place let should be fine, but that's neither here nor there.)






[CLJ-2075] Add three-arities to < <= > >= = == not= Created: 03/Dec/16  Updated: 16/May/17

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

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

Attachments: Text File clj-2075-add-three-arities-to-comparisons-3.patch    
Patch: Code
Approval: Triaged

 Description   

In my practice, using three-arities of less/greater operations is pretty common for e.g. checking a number is in range:

(< 0 temp 100)

The problem is, it is almost three times as slow compared to (and (< 0 temp) (< temp 100)).

This happens because three-arities are handled by the generic vararg arity branch:

(defn <
  "Returns non-nil if nums are in monotonically increasing order,
  otherwise false."
  {:inline (fn [x y] `(. clojure.lang.Numbers (lt ~x ~y)))
   :inline-arities #{2}
   :added "1.0"}
  ([x] true)
  ([x y] (. clojure.lang.Numbers (lt x y)))
  ([x y & more]
    (if (< x y)
     (if (next more)
       (recur y (first more) (next more))
       (< y (first more)))
     false)

This patch adds special handling for three-arities to these fns: < <= > >= = == not=

(defn <
  "Returns non-nil if nums are in monotonically increasing order,
  otherwise false."
  {:inline (fn [x y] `(. clojure.lang.Numbers (lt ~x ~y)))
   :inline-arities #{2}
   :added "1.0"}
  ([x] true)
  ([x y] (. clojure.lang.Numbers (lt x y)))
  ([x y z] (and (. clojure.lang.Numbers (lt x y))
                (. clojure.lang.Numbers (lt y z))))
  ([x y z & more]
   (if (< x y)
     (let [nmore (next more)]
       (if nmore
         (recur y z (first more) nmore)
         (< y z (first more))))
     false)))

The performance gains are quite significant:

(= 5 5 5) 	 24.508635 ns => 4.802783 ns (-80%)
(not= 1 2 3) 	 122.085793 ns => 21.828776 ns (-82%)
(< 1 2 3) 	 30.842993 ns => 6.714757 ns (-78%)
(<= 1 2 2) 	 30.712399 ns => 6.011326 ns (-80%)
(> 3 2 1) 	 22.577751 ns => 6.893885 ns (-69%)
(>= 3 2 2) 	 21.593219 ns => 6.233540 ns (-71%)
(== 5 5 5) 	 19.700540 ns => 6.066265 ns (-69%)

Higher arities also become faster, mainly because there's one less iteration now:

(= 5 5 5 5) 	 50.264580 ns => 31.361655 ns (-37%)
(< 1 2 3 4) 	 68.059758 ns => 43.684409 ns (-35%)
(<= 1 2 2 4) 	 65.653826 ns => 45.194730 ns (-31%)
(> 3 2 1 0) 	 119.239733 ns => 44.305519 ns (-62%)
(>= 3 2 2 0) 	 65.738453 ns => 44.037442 ns (-33%)
(== 5 5 5 5) 	 50.773521 ns => 33.725097 ns (-33%)

This patch also changes vararg artity of not= to use next/recur instead of apply:

(defn not=
  "Same as (not (= obj1 obj2))"
  {:tag Boolean
   :added "1.0"
   :static true}
  ([x] false)
  ([x y] (not (= x y)))
  ([x y z] (not (= x y z)))
  ([x y z & more]
   (if (= x y)
     (let [nmore (next more)]
       (if nmore
         (recur y z (first more) nmore)
         (not= y z (first more))))
     true)))

Results are good:

(not= 1 2 3 4) 	 130.517439 ns => 29.675640 ns (-77%)

I'm also doing what Jozef Wagner did in CLJ-1912 (calculating (next more) just once), although perf gains from that alone are not that big.

My point here is that optimizing three-arities makes sence because they appear in the real code quite often. Higher arities (4 and more) are much less widespread.



 Comments   
Comment by Nikita Prokopov [ 03/Dec/16 2:32 AM ]

Benchmark code here https://gist.github.com/tonsky/442eda3ba6aa4a71fd67883bb3f61d99

Comment by Alex Miller [ 03/Dec/16 8:24 AM ]

It might make more sense to combine this with CLJ-1912, otherwise these patches will fight.

Comment by Nikita Prokopov [ 03/Dec/16 1:02 PM ]

Use this patch if CLJ-1912 would be applied first

Comment by Nikita Prokopov [ 23/Dec/16 7:50 AM ]

I found a problem with previous patches that during defining = (equality), and is not yet defined. Replaced with if

Comment by Alex Miller [ 15/May/17 3:31 PM ]

Dupe of CLJ-1912

Comment by Nikita Prokopov [ 15/May/17 3:43 PM ]

Alex Miller It is a duplicate, but my patch is waaaaaaaaay faster. Just look at the numbers (70-80% improvement vs 5-10%). It’s because I introduced a real arity so that intermediate collection is not created and is not destructured in case of 3 arguments.

Comment by Jozef Wagner [ 15/May/17 11:55 PM ]

There's a quite serious bug in the supplied patch(es), that causes e.g. (= 3 3 2) to return true. Because of this the benchmarks are flawed too I guess.

Comment by Nikita Prokopov [ 16/May/17 3:13 PM ]

Jozef Wagner thanks for spotting this! Attaching an updated path. Benchmark wasn’t flawed too much because perf gain comes not from doing one less/one more comparison but from not having an overhead of calling a fn with unknown arity.





[CLJ-2065] reduce-kv fails on subvec Created: 20/Nov/16  Updated: 19/Sep/17

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

Type: Defect Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File CLJ-2065-reduce-kv-for-SubVector.patch     Text File CLJ-2065-Support-IKVReduce-on-SubVector-2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

reduce-kv works as expected on vectors with the element index passed as the "key" argument. However, it fails with a subvec because clojure.lang.APersistentVector$SubVector does not implement IKVReduce.

(reduce-kv + 0 [1 2 3])
9

(reduce-kv + 0 (subvec [1 2 3] 1))
IllegalArgumentException No implementation of method: :kv-reduce of protocol: #'clojure.core.protocols/IKVReduce found for class: clojure.lang.APersistentVector$SubVector clojure.core/-cache-protocol-fn (core_deftype.clj:583)

One work around is to copy the subvec into a vector:

(reduce-kv + 0 (into [] (subvec [1 2 3] 1)))
6

Note however, the `vec` would not work here. Since Clojure 1.7, vec will return a subvec rather than copying.

(reduce-kv + 0 (vec (subvec [1 2 3] 1)))
IllegalArgumentException No implementation of method: :kv-reduce of protocol: #'clojure.core.protocols/IKVReduce found for class: clojure.lang.APersistentVector$SubVector clojure.core/-cache-protocol-fn (core_deftype.clj:583)

The Clojure user expects that a subvector supports all of the normal vector operations and this exception is confusing. The cause of the problem is that subvec returns a clojure.lang.APersistentVector$SubVector which does not implement clojure.lang.IKVReduce. PersistentVector inherits from APersistentVector and implements IKVReduce but SubVector doesn't get any IKVReduce support. This was probably just an oversight.

There are two patches attached. The first fixed the problem by extending the IKVReduce protocol in core.clj. The second, as suggested by Alex Miller, just adds the Java implementation of the IKVReduce interface directly to APersistentVector$SubVector. The same test is included in both patches.



 Comments   
Comment by Steve Miner [ 20/Nov/16 12:53 PM ]

Here is my current work-around:

(extend-type clojure.lang.APersistentVector$SubVector
  clojure.core.protocols/IKVReduce
  (kv-reduce [subv f init]
    (transduce (map-indexed vector)
               (fn ([ret] ret) ([ret [k v]] (f ret k v)))
               init
               subv)))

In my tests it was usually faster to copy the subvec into a regular vector but I like the look of the transduce fix. It would probably be faster to add a native Java implementation in APersistentVector.java. I'm willing to do the work if the Clojure/core team wants a patch.

Comment by Steve Miner [ 09/Mar/17 1:38 PM ]

Revised work-around using more interop for better performance. Comparable to the speed of normal vector reduce-kv.

(when-not (satisfies?   clojure.core.protocols/IKVReduce (subvec [1] 0))
  (extend-type clojure.lang.APersistentVector$SubVector
    clojure.core.protocols/IKVReduce
    (kv-reduce
      [subv f init]
      (let [cnt (.count subv)]
        (loop [k 0 ret init]
          (if (< k cnt)
            (let [val (.nth subv k)
                  ret (f ret k val)]
              (if (reduced? ret)
                @ret
                (recur (inc k) ret)))
            ret))))))
Comment by Steve Miner [ 18/Sep/17 10:35 AM ]

support IKVReduce for SubVector

Comment by Steve Miner [ 18/Sep/17 10:36 AM ]

Patch also adds test for reduce-kv on regular vector and subvector.

Comment by Alex Miller [ 18/Sep/17 10:48 AM ]

Why not implement IKVReduce directly in APersistentVector$SubVector?

Comment by Steve Miner [ 18/Sep/17 11:33 AM ]

Java implementation of IKVReduce on SubVector

Comment by Steve Miner [ 18/Sep/17 11:37 AM ]

Yes, that makes sense. I hesitated because I didn't want to rework the hierarchy to make SubVector a subclass of PersistentVector but that wasn't necessary to fix the bug. CLJ-2065-Support-IKVReduce-on-SubVector.patch is just the Java side, plus the same test.

Comment by Steve Miner [ 19/Sep/17 7:58 AM ]

Updated patch to inline nth() call, avoiding unnecessary bounds checking.





[CLJ-2054] generator for `any?` occasionally generates `Double/NaN` for which equality semantics don't apply, and that is a problem for the :ret spec of many functions. Created: 07/Nov/16  Updated: 14/Dec/16

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

Type: Enhancement Priority: Major
Reporter: Dimitrios Piliouras Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: generator, spec
Environment:

Ubuntu 16.10 - Oracle Java 8


Approval: Triaged

 Description   

The generator for `any?` will occasionally give back Double/NaN value(s). Since NaNs & equality (via `=`) don't get along, :ret spec'ing a fn which transforms/processes a collection according to a predicate, becomes rather problematic. That's because the most obvious thing to check under :ret (the case where the predicate didn't return true for any value, and so the output coll should be equal to the input coll because nothing was transformed/processed), cannot be expressed trivially.

The workaround I've come up with in my own specs is to spec the elements of the collection with `(s/and any? (complement double-NaN?))` instead of just `any?`, and it works. However, even though I can live without NaNs in the tests, I must admit it still feels sort of hacky.

Ideas:

1) The generator for `any?` could be hardcoded to never return Double/NaN. Sounds rather invasive.
2) The generator for `any?` could be reworked to somehow be configurable wrt allowing/prohibiting Double/NaNs. Then perhaps a dynamic-var and/or a macro (e.g. `without-NaNs`) could expose this (just brainstorming here).
3) The generator for `any? could stay as is, but a new equality operator could be added (e.g. `clojure.spec/===`), which somehow ignores NaNs (a naive implementation for instance might walk the data-structures and replace all NaNs with keywords, and only then perform a regular comparison).



 Comments   
Comment by Alex Miller [ 08/Nov/16 10:29 AM ]

Should consider whether this change is more appropriate in test.check or in the spec generator for any?.

Comment by Dimitrios Piliouras [ 11/Nov/16 12:31 PM ]

It turns out that my workaround does not fully work. I literally just stumbled in the following case:

{nil {[] {NaN 0}}}

which is a conforming value for:

(s/def ::persistent-map
(s/map-of ::anything-but-NaN ::anything-but-NaN)) ;; (s/and any? (complement double-NaN?))

So basically, the inner collections can still have NaNs. So far I've got 4 specs that I've written and faced this problem on all of them.





[CLJ-2049] Improve clojure.zip documentation Created: 25/Oct/16  Updated: 27/Oct/16

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

Type: Enhancement Priority: Minor
Reporter: Brighid M Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, zip
Environment:

All


Attachments: File improve-zip-docs.diff    
Patch: Code
Approval: Triaged

 Description   

The clojure.zip module has extremely terse docstrings that are helpful as reference material but completely unhelpful to someone approaching the module cold. Expanded docstrings would make this piece of code much more visible to users who might find it helpful.



 Comments   
Comment by Brighid M [ 25/Oct/16 9:03 PM ]

A patch that improves clojure.zip's docstrings.

Comment by Alex Miller [ 27/Oct/16 8:42 AM ]

I would prefer if we could minimize the size of the changes by removing diffs that just add periods, change line breaks, add whitespace, or make other non-essential changes. That will help focus on the things that matter like changes in the ns docstring, zipper, etc. Additionally this ticket talks about doc strings but also makes changes in exception messages - I'd prefer code changes to be in a separate ticket. Also, please don't remove the comment sections in the files.

If you would like to convert the test comment section into actual tests, that would be a great separate ticket (I feel like I might have even written a patch to do that at one point but I don't see a ticket for it!).

Keeping this patch entirely focused on essential docstring changes is the best way to ensure its timely inclusion.

Comment by Brighid M [ 27/Oct/16 4:32 PM ]

Alex — to clarify, I'm hearing "this ticket should include two patches: one with the major prose additions and one with small proofreading-ish changes" and "this ticket's patches should not include the changes to exception messages nor the comment-move"?

Supplemental question: is there a style guide hanging around for the code & documentation style of the Clojure core? I poked around for such a guide on clojure.org and in JIRA: I didn't find one, but maybe I overlooked it. I was looking for a guideline about the comment section. It would be good to have a hint about "please don't touch these," because AFAICT they don't communicate that by themselves.

Comment by Alex Miller [ 27/Oct/16 5:43 PM ]

Actually, I'd prefer to have just one patch with the major prose additions. I don't think the minor changes are worth doing and will be a distraction from the more important prose.

Unfortunately, there is no style guide for Clojure core, and in fact it's been written by many people over many years so there generally isn't a consistent style throughout the code. Generally Rich prefers that debug code or comments that trace to him are left intact (git blame can help there). More generally, the simpler a patch is to review, the easier it is for it to stay good and be easily reviewed.

Thanks!

Comment by Brighid M [ 27/Oct/16 6:34 PM ]

> Actually, I'd prefer to have just one patch with the major prose additions.
Okay, I'll produce that patch and attach it to this ticket.

> I don't think the minor changes are worth doing and will be a distraction from the more important prose. Unfortunately, there is no style guide for Clojure core, and in fact it's been written by many people over many years so there generally isn't a consistent style throughout the code.
I think you just made an argument for why it is worth doing minor changes: consistency generally makes things more accessible. However, now is clearly not the time to litigate that, so I'm gonna come back with just the ns/docstring version of this patch.





[CLJ-2040] Allow runtime modification of REPL exception handling Created: 11/Oct/16  Updated: 15/May/17

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

Type: Feature Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: repl

Attachments: Text File CLJ-2040-dynamic-repl-exceptions.patch    
Approval: Triaged

 Description   

Problem Statement

Clojure's REPL is capable of paramterizing almost every aspect of its functionality, including how uncaught exceptions are printed. In the current implementation, these customization hooks are passed in as arguments and closed over, meaning that they cannot be changed once the REPL is started.

Many development tools want to override how the REPL handles uncaught errors. Examples of useful customizations include (but are not limited to):

  • Formatted exception messages (including whitespace and ANSI coloring)
  • Alternative representations for certain types of exceptions (e.g, Spec errors)
  • Dropping into a graphical interaction mode to better inspect ex-data.

Currently, this type of customization must be applied before a REPL is started, meaning that changing how a REPL displays errors requires support from (or plugins to) a third-party tool such as Boot or Leiningen.

Alternatives


1. Take no action.

Third-party tool support is required to create customized exception handling in the REPL. Tools have different techniques for doing this:

  • nREPL can intercept the exception on the wire and passes it through middleware
  • Leiningen plugins alter the root binding of clojure.main/repl-caught.
  • Boot allows users to build a task to invoke clojure.main/repl with the desired arguments.

Users will continue to select one of these according to their tooling preferences.

Benefits:
1. No effort or changes to the existing code.

Tradeoffs:
1. Tools will continue to implement their own diverse, sometimes hacky techniques for printing custom exceptions.
2. Any library intended to provide alternative exception handling will be tied to a specific launcher tool.

2. Make the REPL exception handler dynamically rebindable

If the REPL exception handler were a dynamic, thread-local var, users and libraries could change the behavior of the currently running REPL.

Benefits:
1. Users and libraries can freely override how exceptions are printed, regardless of how Clojure was launched.
2. Fully backwards compatible with existing tools.

Tradeoffs:
1. It will be possible for library authors to provide "bad" or poorly reasoned error printers. This is still possible with launch tools, but the barrier of entry is even lower with libraries.

The attached patch implements this option.

3. Encourage users to start new REPLs instead

In many Clojure environments, it's possible to explicitly launch a REPL from within another REPL. This sub-REPL could have the desired :caught hook.

Benefits:
1. No effort or changes to the existing code.
2. "Functionally pure", and in alignment with the evident design of the current REPL.

Tradeoffs:
1. There is a non-trivial subset of Clojure developers who do not know exactly how REPLs work. They are likely to be confused or subject to increased cognitive load. Insofar as this set of beginner/intermediate developers are precisely who enhanced error messages are meant to help in the first place, this solution is counterproductive.
2. For better or for worse, many existing and widely used tools do not support this. This does not work at all in nREPL, for example. However, even the simplest command-line REPLs behavior would change for the worse; sending a EOF (accidentally or otherwise) would always kill the sub-REPL with no feedback as to what just happened.



 Comments   
Comment by Alex Miller [ 15/Feb/17 9:39 AM ]

On the repl-caught var, it would be good to mention the signature of the handler, namely that it takes an exception, is expected to print or otherwise handle the exception, and that its return will be ignored.

Rather than the changes in repl, what if you added a dynamic-repl-caught and change that to be the default caught handler in repl? Or even just changed repl-caught itself.





[CLJ-2038] Clojure.spec/exercise-fn should accept custom generator map Created: 08/Oct/16  Updated: 07/Nov/16

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

Type: Enhancement Priority: Minor
Reporter: Andrea Richiardi Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec

Attachments: File CLJ-2038-exercise-fn-should-accept-custom-generator.diff    
Approval: Triaged

 Description   

I tried to generate some data with exercise-fn but could not do it because I need to carry my own custom generators.
At the moment though, there is no way to add them, like an optional parameter.



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

Patch would be welcome for this!

Comment by Laszlo Török [ 07/Nov/16 9:23 AM ]

First attempt to form a patch.

I am unsure whether it is ok to use the overloaded 3rd argument approach or take a keyword arg approach for the optional arguments.

i.e. ([sym-or-fn n & {:keys [fspec gen]}])

On the other hand, it only make sense to pass one or the other and given sym-or-fn, fspec-or-gen seems appropriate.

Currently the test suite for c.spec is lacking, I'm happy to add a few example-based tests for exercise-fn, once the approach is approved.
Also





[CLJ-2037] specs in registry lack :file metadata despite having :line, :column Created: 08/Oct/16  Updated: 01/Dec/16

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

Type: Enhancement Priority: Major
Reporter: Felix Andrews Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: spec

Approval: Triaged

 Description   

As of 1.9.0-alpha13, specs in the registry lack :file metadata despite having :line, :column

user=> (require '[clojure.spec :as s])
user=> (-> (s/registry) (get :clojure.core.specs/arg-list) (meta))
{:line 1118, :column 5, :clojure.spec/name :clojure.core.specs/arg-list}
user=> (-> (s/registry) (get 'clojure.core/let) (meta))
{:line 1675, :column 5, :clojure.spec/name clojure.core/let}

This would be useful because:

  • we could list all the specs defined in a project, by filtering the registry.
  • we could read the source of a spec, like clojure.repl/source, for pretty formatting.

(specifically, for use in Codox https://github.com/weavejester/codox/pull/134 )

I had a quick look but couldn't see where the metadata is set.
Cheers



 Comments   
Comment by Alex Miller [ 08/Oct/16 11:12 AM ]

You can use s/describe or s/form to grab the source of a spec now, btw.

Comment by Felix Andrews [ 12/Oct/16 11:29 PM ]

The following works in my tests. (For testing I used in-ns, @#'registry-ref, #'ns-qualify)).

The approach is to set the registry item metadata after a def. It is not enough to set metadata on the def'd value because it is subsequently altered inside def.

(ns clojure.spec)
(alias 'c 'clojure.core)

(defmacro def
  [k spec-form]
  (let [k (if (symbol? k) (ns-qualify k) k)
        m (assoc (meta &form) :file *file*)]
    `(do
       (def-impl '~k '~(res spec-form) ~spec-form)
       (swap! registry-ref update '~k vary-meta c/merge ~m)
       '~k)))

(defmacro fdef
  [fn-sym & specs]
  (let [k (ns-qualify fn-sym)
        m (assoc (meta &form) :file *file*)]
    `(do
       (clojure.spec/def ~fn-sym (clojure.spec/fspec ~@specs))
       (swap! registry-ref update '~k vary-meta c/merge ~m)
       '~k)))

You can use s/describe or s/form to grab the source of a spec now, btw.

Yes, that's nice except for longer specs when line wrapping and indentation would help.

Comment by Jozef Wagner [ 01/Dec/16 12:31 PM ]

Note that current :line and :column meta are not pointing to the place where the spec was defined but to the clojure/spec.clj file, e.g. second example (c.c/let) points to fspec-impl





[CLJ-2025] When a generator fails to gen, state which spec/pred failed Created: 21/Sep/16  Updated: 29/Jan/17

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

Type: Enhancement Priority: Minor
Reporter: David Collie Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec

Attachments: Text File better-such-that-info.patch    
Approval: Triaged

 Description   

Given a generator using such-that that fails to find a value, the error does not give enough information to determine which spec or predicate was at fault:

(require '[clojure.spec :as s])
(s/exercise (s/and string? #{"hi"}))
ExceptionInfo Couldn't satisfy such-that predicate after 100 tries.  clojure.core/ex-info (core.clj:4725)

Another special case of this is when providing a custom generator that produces a valid that doesn't satisfy the spec (Clojure adds this filter internally):

(require '[clojure.spec :as s])
(s/exercise (s/with-gen int? #(s/gen #{:a})))

Proposal: Indicate in the error which spec failed to generate and possibly the path in the overall spec if feasible.

(Note: original description moved to comment)



 Comments   
Comment by Alex Miller [ 22/Sep/16 3:54 PM ]

[Original description from ticket:]

I created a generator that did not conform to the spec (doh!). The generator contained the such-that predicate. When I tried creating a sample from the generator I got this error:

ExceptionInfo Couldn't satisfy such-that predicate after 100 tries.  clojure.core/ex-info (core.clj:4725)

I assumed that it referred to my custom generator but that was a red herring because in fact spec must be using such-that to ensure that the generated value conforms to the spec, and it was this such-that that generated the failure, not the one in my custom generator.

Code (with the problem corrected but showing the such-that in my generator:

(defn mod11-checkdigit
  "Calculate the checkdigit see http://freagra.com/imthealth/mitNNC.html"
  [n]
  (let [x (->> (map #(Integer/parseInt (str %)) (take 9 n))
               (map * (range 10 1 -1))
               (reduce +))
        y (mod x 11)
        c (- 11 y)]
    (cond (== 10 c) nil
          (== 11 c) 0
          :else c)))

(def nhs-number-gen
  "Generates a valid NHS number"
  (gen/fmap #(str (+ (* 10 %) (mod11-checkdigit (str %))))
            (gen/such-that #(mod11-checkdigit (str %))
                           (gen/choose 100000000 999999999))))

(defn nhs-number?
  "Returns true if passed a valid nhs number else returns false"
  [n]
  (and (string? n) (= 10 (count n)) (= (str (mod11-checkdigit n)) (str (last n)))))

(s/def ::nhs-number (s/with-gen nhs-number?
                                (fn [] nhs-number-gen)))

It would be nicer if the error thrown due to the generated value being non-conformant with the spec stated this.

Comment by Alex Miller [ 22/Sep/16 4:07 PM ]

I'm not sure that this is possible right now based on what we give to and get back from test.check.

Comment by Griffin Smith [ 29/Jan/17 6:37 PM ]

It looks like test.check is being updated to support error customization: https://github.com/clojure/test.check/commit/5aea0e275257680b672309b1e940be6dae92c17d . I've got a patch which updates clojure.spec to use it, though obviously it doesn't work since the linked commit hasn't made it into a released version of test.check yet.

Comment by Griffin Smith [ 29/Jan/17 6:40 PM ]

See also http://dev.clojure.org/jira/browse/TCHECK-107 I suppose

Comment by Griffin Smith [ 29/Jan/17 6:51 PM ]

Attached the patch for future reference (`better-such-that-info.patch`).





[CLJ-2015] with-instrument Created: 29/Aug/16  Updated: 15/May/17

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

Type: Feature Priority: Major
Reporter: lvh Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec

Approval: Triaged

 Description   

Right now, instrument and unstrument are great for unconditional instrumentation for tests and for development. I also want to run instrument for just a particular piece of code. For example, I want a test with some stubs or some overrides. Right now I need to instrument and unstrument; I'd prefer to have a with-instrument macro that does the obvious try/finally block for me.



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

So (like most things), obvious things aren't.

There are several ways to call instrument:

  • (instrument)
  • (instrument sym)
  • (instrument [syms])
  • (instrument sym opts)
  • (instrument [syms] opts)

The number there is variable. Similarly, a "body" is typically also variadic in other with-style macros. Parsing those two variadic things is ambiguous.

You mentioned the opts map, so I'm assuming you'd want that as an option. So you could narrow the args to: [sym-or-syms opts & body]. Not sure whether you've then introduced things you don't need in common cases and ruined the usefulness of the macro.

(with-instrument `my-fun {my-opts ...} (test-something))

would expand to

(do
  (instrument user/my-fun)
  (try
    (test-something)
    (finally
      (unstrument user/my-fun))))

There are maybe interesting things to think about with how much you take into account what's already instrumented. Do you unstrument what you instrument, or do you try to return the instrumentation to what it was before (where some stuff may already have been instrumented)?

Comment by Daniel Solano Gómez [ 30/Aug/16 3:24 PM ]

So, here's the implementation I have been using, which isn't necessarily the one to use, but I think it helps with some of the ambiguity with respect to arguments:

(defmacro with-instrumentation
  [args & body]
  `(let [[arg1# arg2#] ~args
         [sym-or-syms# opts#] (cond
                                (nil? arg1#) [(stest/instrumentable-syms) arg2#]
                                (map? arg1#) [(stest/instrumentable-syms) arg1#]
                                :default     [arg1# arg2#])]
     (try
       (stest/instrument sym-or-syms# opts#)
       ~@body
       (finally
         (stest/unstrument sym-or-syms#)))))

It's not perfect, but it has served me well enough.

The question of what happens at the end is a very good one. Ideally, with-instrumentation would have stack-like semantics where instrumentation would return to its previous state. Is that something that can be done with spec?





[CLJ-2013] Alternative s/cat options not error-reported Created: 24/Aug/16  Updated: 04/Oct/17

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

Type: Enhancement Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, spec
Environment:

alpha14


Attachments: Text File CLJ-2013.patch    
Approval: Triaged

 Description   

This problem was detected in context of this discussion https://groups.google.com/d/msg/clojure/mIlKaOiujlo/tF71zZ2BCwAJ

A minimal version of how specs error reporting failed the users intuition there looks like this:

He used an invalid ns form

(ns foo (require [clojure.spec :as s])) ; should be :require

The error reported by spec:

In: [1] val: ((require [clojure.spec :as s])) fails at: [:args] predicate: (cat :docstring (? string?) :attr-map (? map?) :clauses :clojure.core.specs/ns-clauses),  Extra input
:clojure.spec/args  (foo (require [clojure.spec :as s]))
  clojure.core/ex-info (core.clj:4725)

While the error is technically true, it doesn't show the user /how/ each of the alternative options of the reported s/cat failed.

To get a better understanding why the users data is not correct, he should know precisely what spec tried and how it failed.

A good example of how this works is s/alt, where all failing alternatives are always reported to the user.

The problem has been investigated, first experimentally, then in specs code. Finally, a patch that brings error reporting like s/alts comes attached.

It has been observed that specs error reporting behavior for cat with optional branches is the following:

1. If the cat failed after one or many optional branches, the entire cat is reported as failing
2. If the cat failed after one or many optional branches /and/ a subsequent required branch, only the subsequent required branch is reported with no remarks to the alternative optional branches.

Rule 1 explains the ns example.
Rule 2 can fail the users intuition significantly worse:

(s/explain (s/cat :maybe-num (s/? number?)
                  :keyword keyword?)
           ["3"])

gives

In: [0] val: "3" fails at: [:keyword] predicate: keyword?

The report clearly doesn't address the users intent of putting in a number. Instead he is made to believe that he should have entered a keyword.

Solution:

A simple patch has been programmed that changes op-explain to have the following behaviour:

  • All alternatives that have been tried in a s/cat are reported individually.

It improves the reported errors significantly because it makes clearly transparent how the users data failed the validation.

(ns foo (require [clojure.spec :as s])) ; should be :require

now gives

ExceptionInfo Call to clojure.core/ns did not conform to spec:
In: [1] val: (require [clojure.spec :as s]) fails at: [:args :docstring] predicate: string?
In: [1] val: (require [clojure.spec :as s]) fails at: [:args :attr-map] predicate: map?
In: [1 0] val: require fails spec: :clojure.core.specs/ns-refer-clojure at: [:args :clauses :refer-clojure :clause] predicate: #{:refer-clojure}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-require at: [:args :clauses :require :clause] predicate: #{:require}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-import at: [:args :clauses :import :clause] predicate: #{:import}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-use at: [:args :clauses :use :clause] predicate: #{:use}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-refer at: [:args :clauses :refer :clause] predicate: #{:refer}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-load at: [:args :clauses :load :clause] predicate: #{:load}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-gen-class at: [:args :clauses :gen-class :clause] predicate: #{:gen-class}
:clojure.spec/args  (foo (require [clojure.spec :as s]))
  clojure.core/ex-info (core.clj:4725)

It would be even better if explain-data sorted ::s/problems by length of their :path which would push the first two unintended options to the end.

(s/explain (s/cat :maybe-num (s/? number?)
                  :keyword keyword?)
           ["3"])

now gives

In: [0] val: "3" fails at: [:maybe-num] predicate: number?
In: [0] val: "3" fails at: [:keyword] predicate: keyword?

While examples can be made up where this reporting produces more noise (like defn) I believe its the right tradeoff for aforementioned reasons and:

  • We programmers always ask our users for the most specific information when something went wrong - It is correct to apply the same to specs error reporting
  • Custom error reporters (s/explain-out) get more data to generate narrow reports matching the users intent even better


 Comments   
Comment by Nuttanart Pornprasitsakul [ 12/Oct/16 8:43 AM ]

I'll put a somewhat different issue here because I think it has the same root cause.

It should specifically say :ret of fspec is missing but it says failing at :args.

(require '[clojure.spec :as s])
(require '[clojure.spec.test :as st])

(defn x [f] (f 1))

(s/fdef x
  :args (s/cat :f (s/fspec :args (s/cat :i int?))))

(st/instrument `x)

(x (fn [a] a))
Exception in thread "main" clojure.lang.ExceptionInfo: Call to #'user/x did not conform to spec:
In: [0] val: (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"]) fails at: [:args] predicate: (cat :f (fspec :args (cat :i int?))),  Extra input
:clojure.spec/args  (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"])
:clojure.spec/failure  :instrument
:clojure.spec.test/caller  {:file "debug.clj", :line 16, :var-scope user/eval20}
 {:clojure.spec/problems [{:path [:args], :reason "Extra input", :pred (cat :f (fspec :args (cat :i int?))), :val (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"]), :via [], :in [0]}], :clojure.spec/args (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"]), :clojure.spec/failure :instrument,
...
Comment by Leon Grapenthin [ 29/Sep/17 9:07 AM ]

@alexmiller: How about deciding this before releasing 1.9. since it improves error messages in a major way. Since after the original discussion on the groups there have been more reports of unintuitive error messages that this would fix. Related to this is sorting explanations by length of path, as illustrated above (for which there is no ticket yet).
Please get some input from Rich.

Comment by Ghadi Shayban [ 29/Sep/17 9:15 AM ]

Work is ongoing with spec, and is happening in a separate git repo unimpacted by the Clojure 1.9 release. We'll be able to update the spec dependency independent of Clojure and receive an eventual fix.

Comment by Leon Grapenthin [ 29/Sep/17 10:18 AM ]

This affects compile time error messages of Clojure, not just the opt-in part of spec. This part of compiler error reporting is broken and shouldn't be shipped with a major release. In that regard it really doesn't matter whether spec is a separate dep or not.

Also, users who wait for a stable version of spec won't necessarily update their spec dependency.

Comment by Leon Grapenthin [ 30/Sep/17 11:36 AM ]

Also AFAIK better error reporting is the sole reason that Spec is a dependency of 1.9. - please reconsider.

Comment by Alex Miller [ 04/Oct/17 9:01 AM ]

I don't know whether this will be considered before or after 1.9. Seems reasonable based on what I read.

The sorted by path length does have a ticket at CLJ-2063, which was applied in May and released in spec.alpha 0.1.109.





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

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

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

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

 Description   

Minimal example, using 1.9.0-alpha11:

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

The warning is present only if direct linking is disabled.

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

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



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

With DL on:

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

with DL off:

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

bytecode with DL off and current patch:

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




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

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

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

Approval: Triaged

 Description   

In this example a non-conforming value is passed to conform, which should return ::s/invalid but instead throws StackOverflow.

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

CompilerException java.lang.StackOverflowError, compiling:(/Users/alex/code/clojure.spec/src/spec/examples/tree.clj:44:1)
	clojure.lang.Compiler.load (Compiler.java:7415)
	user/eval2674 (form-init3668332544888233146.clj:1)
	user/eval2674 (form-init3668332544888233146.clj:1)
	clojure.lang.Compiler.eval (Compiler.java:6951)
	clojure.lang.Compiler.eval (Compiler.java:6914)
	clojure.core/eval (core.clj:3187)
	clojure.core/eval (core.clj:3183)
	clojure.main/repl/read-eval-print--9692/fn--9695 (main.clj:241)
	clojure.main/repl/read-eval-print--9692 (main.clj:241)
	clojure.main/repl/fn--9701 (main.clj:259)
	clojure.main/repl (main.clj:259)
	clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn--675 (interruptible_eval.clj:69)
Caused by:
StackOverflowError 
	clojure.spec/deriv (spec.clj:1296)
	clojure.spec/deriv (spec.clj:1311)
	clojure.spec/deriv/fn--13794 (spec.clj:1312)
	clojure.core/map/fn--6680 (core.clj:2728)
	clojure.lang.LazySeq.sval (LazySeq.java:40)
	clojure.lang.LazySeq.seq (LazySeq.java:49)
	clojure.lang.RT.seq (RT.java:525)
	clojure.core/seq--6221 (core.clj:137)
	clojure.core/map/fn--6687 (core.clj:2736)
	clojure.lang.LazySeq.sval (LazySeq.java:40)
	clojure.lang.LazySeq.seq (LazySeq.java:49)
	clojure.lang.RT.seq (RT.java:525)


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

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

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

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




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

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

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

Ubuntu Linux 15


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

 Description   

Trying to convert from BigDecimal to long

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

let's just use BigDecimal.longValue()

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

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

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

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

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

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

Patch: clj-2001.patch



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

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

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

Patch seems like it may negatively affect inlining

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

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





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

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

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

Approval: Triaged

 Description   

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

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

Here is a possible replacement:

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

This differs from the existing docstring in two ways:

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

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






[CLJ-1990] Add an async macro that behaves the same as ClojureScript's Created: 24/Jul/16  Updated: 15/May/17

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

Type: Feature Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: clojure.test, portability

Attachments: Text File clj-1990v1.patch    
Approval: Triaged

 Description   

We want to run the same tests between Clojure and ClojureScript. However some of our CLJS tests are asynchronous and require the use of the async macro. Clojure doesn't have the async macro, which makes test portability difficult. If we were to add the async macro, there are at least two possible routes to go down: imitating the async behaviour of cljs.test, or copying the implementation. If we were to imitate the behaviour, we could use the following async macro. It creates a promise, runs the body, blocks on the promise, and done delivers the promise, allowing the test to continue.

(defmacro async
  [done & body]
  `(let [p# (promise)
         ~done #(deliver p# nil)]
     ~@body
     (deref p#)))

This has the advantage that it integrates with the current way tests are run in Clojure, and doesn't require any of the surrounding tooling to be aware of the async testing.

Imitating the implementation would be much more invasive. It would probably mean changing the behaviour of run-tests to return nil like ClojureScript does, and a host of other changes. This means it is likely a non-starter.

I lean heavily towards the first option.

To answer the question "Why do we need this at all when it is a no-op?", this is useful so we can write the same tests in both Clojure and ClojureScript. It would be much more tricky to write without it.



 Comments   
Comment by Daniel Compton [ 26/Jul/16 12:12 AM ]

v1, patch and tests





[CLJ-1982] Better explain reporting on a failed zero or one match with an embedded spec. Created: 18/Jul/16  Updated: 26/Aug/16

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

Type: Enhancement Priority: Minor
Reporter: Nick Jones Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

OSX, Java 8, Clojure 1.9.0-alpha10


Approval: Triaged

 Description   

Problem:

When attempting to validate a vector containing an optional map, the spec will validate correctly if the vector contains a valid map. If however the optional map does not satisfy the spec misleading error messages are produced. It would be nice if on a partial match of an optional map that some indication of this would be given to the user.

Example REPL session to illustrate problem:

The optional nested map (:optional-nested-map) below fails validation because :nested-element-b is a string instead of an int however the explain report says the spec fails at the parent predicate: :user/vector-schema at: [:element-value] predicate: string?.

It would be more helpful for the user in this case if spec reported that the optional nested map at :optional-nested-map had failed due to ::nested-element-b failing the int? predicate.

user=> (require '[clojure.spec :as s])
nil
user=> (s/def ::nested-element-a string?)
:user/nested-element-a
user=> (s/def ::nested-element-b int?)
:user/nested-element-b
user=> (s/def ::nested-element-schema
          (s/keys :opt-un [::nested-element-a ::nested-element-b]))
:user/nested-element-schema
user=> (s/def ::vector-schema
         (s/cat :tag-kw               #{:tag}
                :optional-nested-map  (s/? (s/spec ::nested-element-schema))
                :element-value        string?))
:user/vector-schema
user=> (s/valid? ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b 10} "Element"])
true
user=> (s/valid? ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b ""} "Element"])
false
user=> (s/explain ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b ""} "Element"])
In: [1] val: {:nested-element-a "bla", :nested-element-b ""} fails spec: :user/vector-schema at: [:element-value] predicate: string?
nil
user=>


 Comments   
Comment by Alex Miller [ 18/Jul/16 7:43 AM ]

Can you update this description with a self-contained example that demonstrates the problem? It's too hard to repro and understand this larger example.

Comment by Nick Jones [ 19/Jul/16 3:30 AM ]

Hi,

Sorry I don't seem to have access to edit the description of the ticket after creation. Here is a simplified sample that I hope will help illustrate the case better.

When the optional nested map below fails validation because :nested-element-b is a string instead of an int the explain report says the spec fails at the parent predicate: :user/vector-schema at: [:element-value] predicate: string?.

As it is an optional map I could see how this would be the case. When no match is found it moves onto the next predicate in the parent.

That said I think it could be helpful (especially in a large optional nested data structure) that if a partial match is achieved that that could be reported to the user as a potential spot for the spec failing.

user=> (require '[clojure.spec :as s])
nil
user=> (s/def ::nested-element-a string?)
:user/nested-element-a
user=> (s/def ::nested-element-b int?)
:user/nested-element-b
user=> (s/def ::nested-element-schema
          (s/keys :opt-un [::nested-element-a ::nested-element-b]))
:user/nested-element-schema
user=> (s/def ::vector-schema
         (s/cat :tag-kw               #{:tag}
                :optional-nested-map  (s/? (s/spec ::nested-element-schema))
                :element-value        string?))
:user/vector-schema
user=> (s/valid? ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b 10} "Element"])
true
user=> (s/valid? ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b ""} "Element"])
false
user=> (s/explain ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b ""} "Element"])
In: [1] val: {:nested-element-a "bla", :nested-element-b ""} fails spec: :user/vector-schema at: [:element-value] predicate: string?
nil
user=>
Comment by Nick Jones [ 19/Jul/16 3:45 AM ]

Added simplified version of project archive matching comment at 2016-07-19.

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

Nick, I've given you edit rights here. Generally, we don't like to have external projects for repro - if you can boil it down to a few line example in the description, that would be ideal.

Comment by Nick Jones [ 19/Jul/16 8:15 PM ]

Thanks Alex. I've updated the description and removed the project attachments. I've also added a REPL session to the description to reproduce the problem in a standalone Clojure 1.9.0-alpha10 REPL.





[CLJ-1980] Unable to construct gen in indirectly recursive specs with s/every and derivations Created: 12/Jul/16  Updated: 26/Aug/16

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

Type: Defect Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: spec
Environment:

alpha-10


Approval: Triaged

 Description   

Problem statement: Some spec implementations return no generator but nil, in their gen* implementation when their recursion-limit has been reached (e. g. s/or). Specs that implement composition of other specs sometimes respect getting no generator from other specs gen* and adjust behavior of their own gen* accordingly, sometimes to the extent of returning nothing themselves (e. g. s/or's gen* returns nil if of all of its branches specs also don't have a gen and otherwise uses only those gens it got). However, there are various specs that don't respect getting no generator from gen* (like s/every, s/map-of) and they are essential building blocks in many real world recursive specifications. They then end up throwing an exception "Unable to construct gen ...".

Here is a minimal example (not real world usecase illustration) of the problem with actual specs:

;; A ::B is an s/or with branches going through ::B recursively
(s/def ::B (s/or :A ::A))

;; An ::A is a map of keywords to ::Bs (or it is empty as recursive termination)

(s/def ::A (s/map-of keyword? ::B
                     :gen-max 3))

(gen/sample (s/gen ::A))

ExceptionInfo Unable to construct gen at: [1 :A 1 :A 1 :A 1 :A 1] for: :spec.examples.tree/B  clojure.core/ex-info (core.clj:4725)

Valid values for the spec above (I can mail you a real usecase that enforces above pattern in which we parse an internal query DSL) are: {}, {:a {}}, {:foo {:bar {}}} etc.

The problem why the current implementation of spec fails to generate values for above spec is that ::A's map-of doesn't generate an empty map when ::B's gen* returns nil, but instead throws an exception. s/every and all derived specs are affected by this and there might be others.

Proposed fix: A spec's gen* impl must always respect other spec's gen* returning nil not by throwing but by either adjusting the returned gen or by returning nil itself so that the not-returning-gen behavior propagates back to the caller where an exception should be thrown instead.






[CLJ-1978] recursion-limit not respected Created: 08/Jul/16  Updated: 19/Oct/16

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

Type: Defect Priority: Major
Reporter: Maarten Truyens Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: generator, spec
Environment:

1.9.0-alpha11


Approval: Triaged

 Description   

(Also see closed http://dev.clojure.org/jira/browse/CLJ-1964)

(require '[clojure.spec :as s])
(s/def ::map-tree (s/map-of keyword? (s/or :tree ::map-tree :leaf nil?)))
(s/exercise ::map-tree)

hangs on my machine.

Another example from https://groups.google.com/forum/#!topic/clojure/IvKJc8dEhts, which immediately results in a StackOverflowError on my machine:

(require '[clojure.spec.gen :as gen])

(defrecord Tree [name children])
(defrecord Leaf [name])

(s/def ::name string?)
(s/def ::children (s/coll-of (s/or :tree ::Tree, :leaf ::Leaf)))

(s/def ::Leaf (s/with-gen
                (s/keys :req-un [::name])
                #(gen/fmap (fn [name] (->Leaf name)) (s/gen ::name))))

(s/def ::Tree (s/with-gen
                (s/keys :req-un [::name ::children])
                #(gen/fmap
                   (fn [[name children]] (->Tree name children))
                   (s/gen (s/tuple ::name ::children)))))

;; occasionally generates but usually StackOverflow
(binding [s/*recursion-limit* 1]
    (gen/generate (s/gen ::Tree)))

StackOverflowError 
	clojure.lang.RT.seqFrom (RT.java:533)
	clojure.lang.RT.seq (RT.java:527)
	clojure.core/seq--6221 (core.clj:137)
	clojure.core/map/fn--6687 (core.clj:2736)
	clojure.lang.LazySeq.sval (LazySeq.java:40)
	clojure.lang.LazySeq.seq (LazySeq.java:49)
	clojure.lang.RT.seq (RT.java:525)
	clojure.core/seq--6221 (core.clj:137)
	clojure.core/every? (core.clj:2652)
	clojure.spec/tuple-impl/reify--13509 (spec.clj:905)
	clojure.spec/gensub (spec.clj:228)
	clojure.spec/gen (spec.clj:234)


 Comments   
Comment by Leon Grapenthin [ 12/Jul/16 1:03 PM ]

As the author of CLJ-1964 I can't confirm this.

(binding [s/*recursion-limit* 1]
  (s/exercise ::map-tree))

... immediately generates.

Using the new :gen-max argument spec can also generate with a higher recursion limit in reasonable time

(s/def ::map-tree (s/map-of keyword? (s/or :tree ::map-tree :leaf nil?)
                            :gen-max 3))
(time (s/exercise ::map-tree))
"Elapsed time: 0.135683 msecs"

Note that :gen-max defaults to 20, so with 4 recursion steps this quickly ends up generating 20^5 3.2 million values

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

I tried this again today and the first example still works just fine for me. I'm using Java 1.8 with default settings in a basic Clojure repl (not lein).

Comment by Maarten Truyens [ 19/Oct/16 4:32 AM ]

With the :gen-max option, everything works now. Thanks for the suggestion!





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

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

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

1.9.0-alpha11


Approval: Triaged

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

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



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

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

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

(s/conform
        (s/cat :boxes (s/* #(instance? Box %))
               :name (s/coll-of integer? :kind #(not (record? %))))
        [(Box. 0) [5]])
;;=> {:boxes [#user.Box{:a 0}], :name [5]}




[CLJ-1972] issue with browse-url Created: 28/Jun/16  Updated: 28/Jun/16

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

Type: Defect Priority: Trivial
Reporter: David Siefert Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Check-for-zero-exit-code-to-consider-that-script-exe.patch     Text File 0002-Extracting-method-open-url-by-script-in-browse-url.patch     Text File 0003-Extracting-explaining-method-success-in-open-url-by-.patch    
Patch: Code
Approval: Triaged

 Description   

When xdg-utils are installed on my platform, and the xdg-open command fails, (clojure.java.browse/browse-url) ignores this error and silently fails. This fix will allow the (or ..) logic to continue evaluating to try the next method.






[CLJ-1968] clojure.test/report :error does not flush *out* when the test fails with an exception Created: 23/Jun/16  Updated: 23/Jun/16

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

Type: Defect Priority: Major
Reporter: Sam Roberton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: clojure.test

Approval: Triaged

 Description   

Minimal reproduction:

(require 'clojure.test)

(clojure.test/deftest foo-test
  (throw (ex-info "I fail" {})))

(clojure.test/deftest bar-test
  (.println System/out "bar"))

(clojure.test/test-vars [#'foo-test #'bar-test])

Result:

ERROR in (foo-test) (core.clj:4617)
Uncaught exception, not in assertion.
expected: nil
bar
  actual: clojure.lang.ExceptionInfo: I fail
 at clojure.core$ex_info.invokeStatic (core.clj:4617)
...

Note "bar" appearing in the output in the middle of the error report for foo-test.

Analysis:

(clojure.test/report {:type :error, :actual some-exception}) calls stack/print-cause-trace. Unlike other clojure.test/report callpaths, this does not flush on newline. Thus, when tests fail with exceptions and there is anything writing directly to Java's System.out, there can be a large gap between the first part of the error report and the exception trace.

(To explain why this is annoying: we're running Selenium tests via clj-webdriver, and our system under test is logging with log4j via clojure.tools.logging. We invariably see dozens or even hundreds of lines between "expected: ..." and the subsequent "actual: ..." exception trace. This makes it very easy to come to completely the wrong conclusion about when failures occurred with respect to the other events that appear interleaved in the log.)

It would be preferable (in my opinion) if clojure.test/report always constructed the output from each individual invocation into a single string which got written to *out* all at once – that way there could be no way for output to be interleaved from other threads. Absent that, it would at least help a lot if the :error implementation called (flush).






[CLJ-1960] Bug in clojure.core/mod with large Double argument Created: 14/Jun/16  Updated: 15/Jun/16

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

Type: Defect Priority: Minor
Reporter: William Tozier Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math, numerics
Environment:

Java 8 update 91 on Mac OS X 10.11.5


Approval: Triaged

 Description   

The `clojure.core/mod` function works just as expected for small positive floating-point dividend and small positive integer divisor. But today I was working on some edge case tests and came across the following inexplicable behavior:

REPL_session
user=> (def big  Double/MAX_VALUE)
#'user/big
user=> (mod big 10)
0.0
user=> (mod big 100)
0.0
user=> (mod big 1000)
1.9958403095347198E292
user=> (mod big 999)
-Infinity
user=> (mod big 998)
0.0
user=> (mod big 997)
1.9958403095347198E292
user=> (mod big 996)
0.0
user=> (mod big 995)
0.0
user=> (mod big 994)
0.0
user=> (mod big 1001)
1.9958403095347198E292
user=> (mod big 1002)
0.0
user=> (mod big 1003)
0.0
user=> (mod big 1004)
-Infinity
user=> (mod big 1005)
0.0

No idea whether this is inherited from a Java bug. I can see nothing special about the values chosen, and I suspect if one scanned it'd be easy to find other glitches.



 Comments   
Comment by Alex Miller [ 14/Jun/16 7:12 PM ]

mod is based on rem - from a glance, mod does not seem to account properly for any case of overflow, and I suspect that's at the root of a lot of these problems.

Comment by Gary Fredericks [ 14/Jun/16 7:15 PM ]

Test.check suggests (mod 6.7772677936779424E16 23) => -8.0 is somewhat close to minimal.

Comment by William Tozier [ 15/Jun/16 12:40 PM ]

Actually, just checked, and rem gives the same results. Thus (rem Double/MAX_VALUE 1001) is 1.9958403095347198E292, and (rem 6.7772677936779424E16 23) => -8.0.





[CLJ-1959] adding functions `map-vals` and `map-keys` Created: 14/Jun/16  Updated: 17/Nov/17

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

Type: Feature Priority: Major
Reporter: Hiroyuki Fudaba Assignee: Unassigned
Resolution: Unresolved Votes: 42
Labels: None

Attachments: Text File map-mapper.patch     Text File map-mapper-v2.patch     Text File map-mapper-v3.patch    
Approval: Triaged

 Description   

Many people have been writing a function to map values in HashMap:

Proposal: Add `map-keys` and `map-values` which: maps keys in HashMap, and map values in HashMap. They return HashMap as a result.

Workaround: Using function `reduce-kv` or ordinary `map` and `into` is a common solution, but they are confusing and types change, which makes it tricky and tedious.

Discussions: https://groups.google.com/forum/#!topic/clojure-dev/kkPYIl5qj0o



 Comments   
Comment by Hiroyuki Fudaba [ 14/Jun/16 11:22 AM ]

code and test for map-keys and map-vals

Comment by Nicola Mometto [ 14/Jun/16 1:05 PM ]

I propose those functions being called `update-vals` and `update-keys` rather than `map-vals` and `map-keys`

Comment by Alex Miller [ 14/Jun/16 2:03 PM ]

It's not worth bike-shedding names on this - Rich will have his own opinion regardless.

On the patch:

  • remove the :static metadata, that's not used anymore
  • needs docstrings, which should be written in the style of other Clojure docstrings. map is probably a good place to draw from.
  • rather than declare into, defer the definition of these till whatever it needs has been defined. There is no reason to add more declares for this.

There are other potential implementations - these should be implemented and compared for performance across a range of input sizes. In addition to the current approach, I would investigate:

  • reduce-kv with construction into a transient map. This allows the map to reduce itself (no seq caching needed) and avoid creating entries only to tear them apart again.
  • transducers with (into {} (map ...) m)

Also should consider

  • whether to build a k/v vector and convert to a map, or build a map directly (the former may be faster, not sure)
  • if building the map, how to construct the map entries (vector vs creating a mapentry object directly)
  • in map-keys, is there any open question when map generates new overlapping keys?
  • are there places in existing core code where map-keys/map-vals could be used (I am pretty certain there are)
Comment by Hiroyuki Fudaba [ 15/Jun/16 11:01 AM ]

Thanks for comments

> I propose those functions being called `update-vals` and `update-keys` rather than `map-vals` and `map-keys`
Maybe. But I name it `map-*` just for now, we can choose it later

about potential implementations:
I have tried several implementations, and seems to be the current implementation is the fastest.
You can see it here: https://github.com/delihiros/performance

about considerings:
> whether to build a k/v vector and convert to a map, or build a map directly (the former may be faster, not sure)
> are there places in existing core code where map-keys/map-vals could be used (I am pretty certain there are)
> if building the map, how to construct the map entries (vector vs creating a mapentry object directly)
I'll check which them as soon as possible. I haven't done it yet.

> in map-keys, is there any open question when map generates new overlapping keys?
I believe it should be overwritten by latter applied key and value.

Comment by Nathan Marz [ 15/Jun/16 11:35 AM ]

I've done quite a bit of investigation into this through building Specter. Here are some benchmarks of numerous ways of doing map-vals, including using Specter.

Code: https://github.com/nathanmarz/specter/blob/4778500e0370fb211f47ebf4d69ca64366117b6c/scripts/benchmarks.clj#L87
Results: https://gist.github.com/nathanmarz/bf571c9ed86bfad09816e17b9b6e59e3

A few comments:

  • Implementations that build and tear apart MapEntry's perform much worse.
  • Transients should be used for large maps but not for small ones.
  • This benchmark shows that the property of maintaining the type of the map in the output can be achieved without sacrificing performance (the test cases using Specter or "empty" have this property).
Comment by Hiroyuki Fudaba [ 11/Jul/16 3:27 AM ]

I've modified the implementation. It should be faster than before.

Comment by Steve Miner [ 20/Jul/16 10:46 AM ]

Implementations that call reduce-kv are not lazy so the documentation should be clarified in the proposed patch (map-mapper-v3.patch). Also, it's probably better to say "map" (as the noun) rather than to specify a particular concrete type "hash map".

Comment by Nicola Mometto [ 21/Jul/16 4:30 AM ]

map->map operations can't be lazy either way. Even if one implementation used lazy operations to iterate over the original map, the `into {}` would realize it later.

Comment by Nathan Marz [ 14/Nov/17 2:15 PM ]

-1 to this. Clojure aims to be a small core, pushing additional functionality into libraries. The problem space of compound transformations, of which this functionality is a small piece, is already thoroughly solved by Specter. Specter's `MAP-VALS` and `MAP-KEYS` navigators additionally support removal of key/value pairs during transformation by transforming to special `NONE` value. This expands the utility greatly.

Also worth noting is a fast implementation requires a totally different approach depending on the type of the map. `reduce-kv` with transients is optimal for hash maps, but for array maps using lower level facilities provide ~60% speed boost. See Specter's implementation here https://github.com/nathanmarz/specter/blob/1.0.4/src/clj/com/rpl/specter/navs.cljc#L243

Comment by Ghadi Shayban [ 14/Nov/17 4:40 PM ]

Nathan you're making a strawman re: compound transformations. This isn't a request for a function with filtering knobs or conditional behavior (which Clojure has historically opposed). There are other valid approaches than Specter's.

Re fast implementation: Not every function has to strive for the most performant implementation, esp at the cost of branching and general complexity. A cost model for performance has to take into account complexity.

This ticket is a request for a convenience that is repeated in many codebases.

Do we want to preserve metadata? Many map operations do.
Do we want to assume IEditableCollection?

Comment by Nathan Marz [ 14/Nov/17 8:57 PM ]

Performance is incredibly important for general data structure manipulation functions like this. Manipulating data structures is one of the most basic things you do in a language, and it's done all the time in performance sensitive code. Having to abandon the "nice" functions for ugly specialized code in performance sensitive code is a failure of the language.

`map-vals`/`map-keys` are part of a rich problem space of which myself and the users of Specter have learned a lot the past few years. Clojure barely touches this problem space, especially when it comes to nested or recursive data structures. Adding functions to Clojure that are significantly inferior in both semantics and performance to what already exists in an external library seems pretty silly to me. Just my two cents.

Comment by Hiroyuki Fudaba [ 17/Nov/17 6:48 AM ]

I agree with Nathan Martz that the performance is very important, but I still have a strong opinion that this function should be somehow imported to the core part of the language.
People use this transformation pretty often and if there is a fast implementation in the core it will be a great benefit to all of us.





[CLJ-1955] .hashCode throws ClassCastException when called on some functions Created: 09/Jun/16  Updated: 01/Dec/16

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

Type: Defect Priority: Minor
Reporter: Georgi Danov Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

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

 Description   
user> some?
#function[clojure.core/some?]
user> (.hashCode map)
72400056
user> (.hashCode str)
ClassCastException clojure.core$str cannot be cast to java.lang.String  /eval39172 (form-init3428514420830954023.clj:5793)
user> (.hashCode (fn []))
1715179801
user> (.hashCode some?)
ClassCastException clojure.core$some_QMARK_ cannot be cast to java.lang.Boolean  /eval39178 (form-init3428514420830954023.clj:5797)
user> (.hashCode #'some?)
1955712430
user> (.hashCode @#'some?)
1726569843


 Comments   
Comment by Nicola Mometto [ 10/Jun/16 3:27 AM ]

This happens because `some?` and `str` have type hints on the Var to signal the type returned by their invocations, but the Compiler thinks those type hints apply to the Var object itself aswell.

An easy fix would be to move those type hints from the Var (old-style) to the argvec (new-style)

Comment by Ghadi Shayban [ 14/Jun/16 3:36 PM ]

agreed with nicola's suggestion - change type hints. This is a dup of CLJ-140 where :tag causes confusion when a var is being invoked vs used in expr context

Comment by Jozef Wagner [ 01/Dec/16 1:29 PM ]

Patch attached





[CLJ-1950] cl-format is too slow for production use Created: 05/Jun/16  Updated: 05/Jun/16

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

Type: Enhancement Priority: Minor
Reporter: Alain Picard Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: performance, print
Environment:

Mac OS X - 3GHz i7 16Gb ram


Approval: Triaged

 Description   

Run this example code:

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
(in-ns 'clojure.pprint)

(println "Basic output using raw str.")
(time
(doseq [i (range 1000)]
(apply str (interpose "," [1 2 3]))))

(println "Test 1 - raw cl-format")
(time
(doseq [i (range 1000)]
(clojure.pprint/cl-format nil "~{D^,~}" [1 2 3])))
;; ==> "Elapsed time: 231.345 msecs"

(println "Test 2 - call on the compiled format")
(def myx
(compile-format "~{D^,~}"))

(time
(doseq [i (range 1000)]
(clojure.pprint/cl-format nil myx [1 2 3])))

(println "Test 3 - using a formatter")
(def myy
(formatter "~{D^,~}"))

(time
(doseq [i (range 1000)]
(myy nil myx [1 2 3])))

(time
(dotimes (i 100000)
(format nil "~{D^,~}" '(1 2 3))))

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

It will print something like this:

Basic output using raw str.
"Elapsed time: 2.402 msecs"
Test 1 - raw cl-format
"Elapsed time: 194.405 msecs"
Test 2 - call on the compiled format
"Elapsed time: 87.271 msecs"
Test 3 - using a formatter
"Elapsed time: 199.318 msecs"

So raw `str' is ~ 100X faster.

For reference, on the same hardware, using
SBCL Common Lisp, this test runs in < 1 ms.

There are (at least) 2 problems here:

1. cl-format function begins with a line like:

let [compiled-format (if (string? format-in) (compile-format format-in) format-in)

But there is no api to pass in a compiled-format into it; (as compile-format
is a private function, so can't be used at large) so this is kind of useless.

2. Even using a precompiled formatter is way too slow.

Suggested fix: none, except perhaps warning unwary users that this
function is simply not suitable for tight loops, and should only be
used to pretty print user input strings, etc.

Thank you






[CLJ-1949] Generator for fspec is not deterministic & ignores sizing Created: 05/Jun/16  Updated: 26/Aug/16

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

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

Attachments: Text File CLJ-1949-impure.patch     Text File CLJ-1949-pure.patch    
Patch: Code
Approval: Triaged

 Description   

Problem

One of the goals of test.check is for users to be able to write arbitrarily rich generators while maintaining determinism, which has obvious benefits for reproducing failures.

Currently the fspec generator generates a function which itself generates random return values by calling clojure.test.check.generators/generate, which is a function intended only for development use as it circumvents test.check's controlled source of psuedorandomness. It also circumvents test.check's sizing mechanism, since the generate function always uses a size of 30.

Possible Solutions

I see two reasonable solutions to this, depending on whether the generated function ought to be a pure function (which it currently isn't, since it ignores its arguments and randomly generates a return value).

Pure Function

We can generate a non-empty vector of possible return values and use that to create a function that selects one of the possible return values using the hash of the arguments.

Impure Function

We can generate a non-empty collection of possible return values and use that to create a function with internal state that cycles through the possible return values.



 Comments   
Comment by Gary Fredericks [ 05/Jun/16 5:44 PM ]

Added a patch for each of the approaches listed. Would be happy to add tests too if feedback is given about either approach being preferred.





[CLJ-1929] Can't typehint literal collection to avoid reflection on Java interop call Created: 16/May/16  Updated: 18/May/16

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

Type: Defect Priority: Major
Reporter: David Bürgin Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: interop, reflection, typehints
Environment:

OS X 10.11.4


Attachments: Text File 0001-CLJ-1929-preserve-type-hints-in-literals.patch    
Patch: Code
Approval: Triaged

 Description   

There is a reflection warning when passing a Clojure collection to a method that has a parameter of a collections interface type like java.util.Map.

Example calling java.time.format.DateTimeFormatterBuilder.appendText(java.time.temporal.TemporalField, java.util.Map):

(import 'java.time.format.DateTimeFormatterBuilder
        'java.time.format.TextStyle
        'java.time.temporal.ChronoField)

(set! *warn-on-reflection* true)

(let [builder (DateTimeFormatterBuilder.)]
  (.appendText builder ChronoField/YEAR {}))
; Reflection warning, NO_SOURCE_PATH:6:3 - call to method appendText on java.time.format.DateTimeFormatterBuilder can't be resolved (argument types: java.time.temporal.ChronoField, clojure.lang.IPersistentMap).

The map literal cannot be hinted:

(let [builder (DateTimeFormatterBuilder.)]
  (.appendText builder ChronoField/YEAR ^java.util.Map {}))
; Reflection warning, NO_SOURCE_PATH:8:3 - call to method appendText on java.time.format.DateTimeFormatterBuilder can't be resolved (argument types: java.time.temporal.ChronoField, clojure.lang.IPersistentMap).

The warning does not appear when the map is not empty:

(let [builder (DateTimeFormatterBuilder.)]
  (.appendText builder ChronoField/YEAR {1 "a"}))

Nor does it appear on similar methods where there is no overloaded method with the same arity:

(let [builder (DateTimeFormatterBuilder.)]
  (.appendZoneText builder TextStyle/FULL #{}))

Workaround is to not use a literal:

(let [builder (DateTimeFormatterBuilder.)]
  (.appendText builder ChronoField/YEAR ^java.util.Map (array-map)))

It should be possible to infer in these cases like elsewhere that {} implements java.util.Map.

If that is not viable a type hint on {} should be honored.

Approach: preserve user hints in literal collections
Patch: 0001-CLJ-1929-preserve-type-hints-in-literals.patch






[CLJ-1915] Tests for clojure.core/atom Created: 18/Apr/16  Updated: 18/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Linus Ericsson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: atom

Attachments: File 0001-atom-unit-tests.clj    
Patch: Code and Test
Approval: Triaged

 Description   

As per discussion with Alex Miller Mars 3rd 2016 on clojure-dev, Alex suggested we should add tests to clojure.core/atom functionality, of which there is none today.

I proposed tests for

  • the various ways to instatiate atoms (with and without validator and metadata)
  • that validators throws correctly
  • adding and removing watchers and that they trig as one would expected.
  • various ways of changing values (no aim for finding high-load concurrency issues or patological cases or similar).
  • that the arities of the interface in IAtom .swap works as expected - ie no reflection warnings (help/pointers for these type of cases needed!)
  • generative, tests trying to find the glitches while using atoms (the things excluded above).

Alex suggested generative testing, but no performance tests.

The patch "0001-atom-unit-tests.clj" (attached) contains unit tests for

  • creating "bare" atom
  • creating atom with validator
  • that validate-fn triggers and that the atom is unchanged
  • that deref (@) reader macro creates correct '(clojure.core/deref a)
  • that CAS works for ordinary values (no validator-triggering etc).

There are plenty of combinations not covered with these tests, but this is a start.

To cover all cases (like cas-ing with invalid values and other strange things) generative testing is indeed a must.






[CLJ-1911] min-key and max-key should return NaN if any of the argument is NaN Created: 08/Apr/16  Updated: 12/May/16

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

Type: Defect Priority: Minor
Reporter: Renzo Borgatti Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None
Environment:

Likely All. Including older version of Clojure.


Attachments: Text File CLJ-1911-contagious-NaN-and-tests.patch     Text File CLJ-1911-contagious-NaN.patch     Text File CLJ-1911-NaN-fix-over-CLJ-99.patch    
Patch: Code
Approval: Triaged

 Description   

It appears that min-key and max-key behave incorrectly (following Java that follows IEEE floating point convention):

(apply max-key last [[:a 10000] [:b (/ 0. 0)] [:c 0]])
[:c 0]

Not sure how this should then propagate forward, but definitely not silently. Options:

1. [:b NaN] (the first item to generate the NaN)
2. NaN (this is changing the expected type)
3. ArithmeticException Operation with at least one NaN operand.

If this was to be patched the same as it was for min/max (http://dev.clojure.org/jira/browse/CLJ-868) it will probably result in option 1.



 Comments   
Comment by Nicholas Antonov [ 14/Apr/16 9:36 PM ]

This implements the first solution of a contagious NaN in the same style as CLJ 868

Comment by Alex Miller [ 15/Apr/16 12:03 AM ]

Patch should have tests...

Comment by Nicholas Antonov [ 15/Apr/16 1:07 AM ]

This latest patch adds tests for min-key and max-key with and without NaN results, as there were none before.

Comment by Alex Miller [ 29/Apr/16 10:06 AM ]

This overlaps with CLJ-99, which has already been prescreened. I would like to base whatever changes this patch requires over the top of that ticket. To build this, apply the CLJ-99 patch, then branch, make you changes, and then create a patch relative to the clj-99 branch. Sorry that's a pain - usually patches don't collide at this level of conflict.

Comment by Nicholas Antonov [ 12/May/16 6:14 AM ]

The latest patch fixes min and max key in the same way, but based over CLJ-99, only evaluating the function once for each item.





[CLJ-1907] Document non-caching behaviour of `iterate` when used as generator Created: 31/Mar/16  Updated: 31/Mar/16

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

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

Approval: Triaged

 Description   

The non-caching behaviour of `iterate` when used as a generator is not documented and counter-intuitive. It should be documented, just like it's documented for e.g. `eduction`.

Even though the docstring for `iterate` requires `f` to be side-effect free, `f` might take a long time to compute, in which case users should be wary that the computation might happen more than once.






[CLJ-1898] Inconsistent duplicate check in set/map literals with quoted/unquoted equal constants Created: 06/Mar/16  Updated: 01/Dec/16

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections, compiler

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

 Description   

Set and map literals containing the same constant quoted and unquoted, will throw a duplicate key exception in some cases (the correct behaviour), while silently ignore the duplicate in some others.

user=> #{'1 1}
#{1}
user=> #{'[] []}
IllegalArgumentException Duplicate key: []  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:56)

This happens because the compiler assumes that literals that have distinct elements at read-time, will have distinct elements at runtime. This is not true for self-evaluating elements where (quote x) is equal to x



 Comments   
Comment by Jozef Wagner [ 01/Dec/16 3:38 PM ]

Attached patch with tests.





[CLJ-1896] Support transducers in vec and set fns Created: 24/Feb/16  Updated: 15/May/17

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

Type: Feature Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: transducers

Approval: Triaged

 Description   

Rather than

(into [] (map inc) [1 2 3])
vec (and set) could support the transducer directly:

(vec (map inc) [1 2 3])
(set (map inc) #{1 2 3})

Depending how far we wanted to take this, the implementation could be somewhat clever for vec in building the initial set of results in an array and then creating the vector with it directly as is already done in some other cases.






[CLJ-1890] enhance pprint to print type for defrecord (as in pr) Created: 05/Feb/16  Updated: 06/Nov/17

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

Type: Feature Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: pprint, print

Attachments: Text File CLJ-1890-pprint-records-2.patch    
Approval: Triaged

 Description   

pprint currently doesn't print the names of defrecord types, instead printing just the underlying map. This is in contrast to pr-str/println. This ticket proposes that the behaviour of pprint is changed to match pr-str and println's.

More discussion at https://groups.google.com/forum/#!topic/clojure-dev/lRDG6a5eE-s

user=> (defrecord myrec [a b])
user.myrec
user=> (->myrec 1 2)
#user.myrec{:a 1, :b 2}
user=> (pr-str (->myrec 1 2))
"#user.myrec{:a 1, :b 2}"
user=> (println (->myrec 1 2))
#user.myrec{:a 1, :b 2}
nil
user=> (pprint (->myrec 1 2))
{:a 1, :b 2}
nil

Approach: Add new IRecord case to pprint's simple-dispatch mode. Extract guts of pprint-map to pprint-map-kvs and call it from both existing pprint-map and new pprint-record. Set multimethod preference for IRecord version. Added test.

user=> (pprint (->myrec 1 2))
#user.myrec{:a 1, :b 2}

Patch: CLJ-1890-pprint-records-2.patch

Prescreened by: Alex Miller

Also see: CLJS-1753



 Comments   
Comment by Daniel Compton [ 05/Feb/16 1:51 PM ]

The fix for this will needed to be ported to ClojureScript too.

Comment by Steve Miner [ 06/Feb/16 11:55 AM ]

Added patch to pprint records with classname.

Comment by Steve Miner [ 06/Feb/16 12:03 PM ]

Open question: How should pprint handle a record that has a print-method defined for it? Should the print-method be used instead of the pprint default?

The current release and my patch do not consider the print-method when calling pprint.

Comment by Alex Miller [ 08/Feb/16 4:33 PM ]

I do not think pprint should check for or use print-method. pprint has it's own simple-dispatch multimethod that you can extend, or it will ultimately fall through to pr (which can be extended via print-dup).

Example of the former:

user=> (defrecord R [a])
user=> (def r (->R 1))
user=> (pprint r)
{:a 1}

user=> (use 'clojure.pprint)
user=> (defmethod simple-dispatch user.R [r] (pr r))
#object[clojure.lang.MultiFn 0x497470ed "clojure.lang.MultiFn@497470ed"]
user=> (pprint r)
#user.R{:a 1}
Comment by Steve Miner [ 09/Feb/16 6:27 AM ]

Right, clojure.pprint/simple-dispatch is there for user code to customize pprint, independent of whatever they might do with print-method. No need to conflate the two. So the patch is ready to review.

Comment by Michael Blume [ 05/Nov/17 1:51 AM ]

Patch does not appear to apply cleanly to current master.

Comment by Michael Blume [ 05/Nov/17 1:53 AM ]

Doesn't apply cleanly to any of the beta tags, does apply cleanly to 1.8.0, but can't rebase without a conflict. Steve, you should probably fix that.

Comment by Steve Miner [ 05/Nov/17 8:00 AM ]

revised patch for current master (Clojure 1.9 beta4)

Comment by Steve Miner [ 05/Nov/17 8:12 AM ]

The old patch was prescreened but I guess the new patch should be reconsidered from scratch as namespaced maps were added to Clojure in between patches. By the way, I don't think there are any tests to cover pretty-printing namespaced maps. The only test I could find was for pr-str of a namespaced map. In any case, I tried to make sure the prefix logic was maintained so it should still work. The same test for pretty-printing records is used in both versions of the patch.





[CLJ-1889] Add optional predicate to string trim functions that determines if a character should be trimmed Created: 27/Jan/16  Updated: 15/May/17

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

Type: Feature Priority: Minor
Reporter: Tamas Szabo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: string

Attachments: Text File CLJ-1889-trim-enhancement.patch    
Approval: Triaged

 Description   

The proposal is that the trim functions (trim, triml, and trimr) would get a second arity with a function trim?:

[trim? ^CharSequence s]

trim? comes first to support partial.

New doc string would be:

"Removes characters from both ends of string. 
 If trim? is omitted white space is removed. When supplied it accepts 
 a character and returns true if the character should be removed."

Example test:

(deftest t-trim 
  (is (= "foo" (s/trim "  foo  \r\n"))) 
  (is (= "bar" (s/trim "\u2000bar\t \u2002"))) 

  ;; Additional test 
  (is (= "bar" (s/trim "$%#\u2000bar\t \u2002%$#" 
                       #(or (Character/isWhitespace %) ((set "$#%") %))))))

Similar to Python's strip - https://docs.python.org/2/library/stdtypes.html#str.strip

Approach: The proposed solution isn't very DRY but it follows the design guidelines at the top of the file, more exactly point 3:

"3. Functions take advantage of String implementation details to
write high-performing loop/recurs instead of using higher-order
functions. (This is not idiomatic in general-purpose application
code.)"

First I had a solution in which I replaced Character/isWhitespace from the current implementation by calling pred. pred was defaulted to an is-whitespace? function.
That code is of course nicer, even trim-newline could just call into trimr, removing a lot of duplication, but it adds the overhead of always calling a function, instead of calling Character/isWhitespace directly.

The only way I can see to have optimised and DRYer code is to use macros, but I don't think that it will necessary lead to nicer code.

Given the existing design style of the other functions in string.clj I felt that the best solution would be to just simply duplicate in favour of optimised code.



 Comments   
Comment by Tamas Szabo [ 27/Jan/16 2:44 PM ]

Proposed solution. Code + tests.

Comment by Tamas Szabo [ 27/Jan/16 3:42 PM ]

Added new patch that renames pred to trim?

Comment by Andy Fingerhut [ 27/Jan/16 6:27 PM ]

Note that Java, and thus Clojure/Java, uses UTF-16 encoding for strings in memory. Thus if you wanted to trim a set of Unicode code points from the beginning and/or end of a string, the API of trim? taking a single 16-bit Java character is not enough information to determine whether it should be trimmed or not.

If you want to handle that generality, it would require a more complex implementation, which checks whether the first/last character is one half of a code point that is encoded as 2 16-bit Java characters, and pass a 32-bit int to trim?, or something similar to that.

I have no objections if these API enhancements are made without enabling testing against an arbitrary Unicode code point. In the past, similar suggestions have been rejected in Clojure's built-in lib, e.g. CLJ-945

Comment by Tamas Szabo [ 28/Jan/16 1:56 AM ]

Yes, the UTF-16 encoding and Character representing either a codepoint or a half-codepoint is a bit of a mess, isn't it?

In the Java String and Character API's the methods that accept char, handle only characters in the Basic Multilingual Plane.
trim? accepts a character, so following the same behavior it will work only for removing characters in the Basic Multilingual Plane.

I think even this would be fine, but additionally because the high/low surrogates and the BMP characters are disjoint, you could actually use the same implementation to remove Unicode code points that aren't in the BMP. You can just say that both the high and low code unit of the codepoint are "unwanted".

Ex:
𝄞 is "\uD834\uDD1E"

user=> (trimr (set " \uD834\uDD1E") "example string  𝄞  ")
"example string"
Comment by Andy Fingerhut [ 28/Jan/16 5:11 AM ]

Agreed, but probably better to anti-recommend such an implementation of trimr for removing such things, because it would also remove only one UTF-16 Java character out of 2 high/low surrogates if it matched a member of the set, even if the other surrogate didn't match anything in the set, which would leave behind a malformed UTF-16 string.

Again, probably best to either not include this in the implementation at all, and at most warn about it in the docs, or to handle it in the implementation by checking for high/low surrogates in the loop(s).

Comment by Tamas Szabo [ 28/Jan/16 6:00 AM ]

Yes, you're right. That solution won't work in all cases, so it can't be recommended.

I am slightly inclined towards having trim? accept chars and work only for removing BMP characters. This will arguably be enough for the majority of the use cases.
The other solution can be used for all use cases, but then trim? will have to accept int, or 2 chars, or a string, so trim? would be less intuitive (although closer to the real world ), and writing those trim? functions would be less user friendly.

That being said, I am happy to change the implementation to do that if it is required.

Currently, I'm not even sure if the enhancement will be accepted or rejected or what the process for that is.





[CLJ-1885] data/diff does not return a tuple when comparing different maps Created: 16/Jan/16  Updated: 16/Jan/16

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

Type: Defect Priority: Minor
Reporter: Eric Dvorsak Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

all


Attachments: Text File CLJ-1885.patch     Text File CLJ-1885-tests.patch    
Approval: Triaged

 Description   

Problem: clojure.data/diff inconsistently returns a lazy seq when comparing different maps, but a vector otherwise.

user> (data/diff {:a 1 :b 2} {:a 1})
({:b 2} nil {:a 1})

This is inconsistent with doc and normal behavior :

user> (data/diff {:a 1 :b 2} {:a 1 :b 2})
[nil nil {:a 1, :b 2}]
user> (data/diff #{1 2 3} #{1 2 3})
[nil nil #{1 3 2}]
user> (data/diff #{1 2 3} #{1 2})
[#{3} nil #{1 2}]

The docstring states: "Recursively compares a and b, returning a tuple of [things-only-in-a things-only-in-b things-in-both]", implying that it should always return a vector.



 Comments   
Comment by Eric Dvorsak [ 16/Jan/16 10:02 AM ]

Fixing it just requires to vectorize diff-associative output like this :

(defn- diff-associative
  "Diff associative things a and b, comparing only keys in ks."
  [a b ks]
  (vec (reduce
   (fn [diff1 diff2]
     (doall (map merge diff1 diff2)))
   [nil nil nil]
   (map
    (partial diff-associative-key a b)
    ks))))
Comment by Alex Miller [ 16/Jan/16 10:10 AM ]

There are other potential ways to address this, such as by using transducers instead. Not sure if that's worth doing, but seems reasonable to consider while we're making changes.

Comment by Eric Dvorsak [ 16/Jan/16 10:15 AM ]

Maybe this could be done as an improvement and proposed in an other ticket.

Vec is already used to vectorize the lists in diff-sequential. I would suggest to just fix the bug and add the test cases that should have screen it.

Comment by Eric Dvorsak [ 16/Jan/16 10:20 AM ]

There is a test case that should already fail :

[{:a #{2}} {:a #{4}} {:a #{3}}] {:a #{2 3}} {:a #{3 4}}

I get

({:a #{2}} {:a #{4}} {:a #{3}})
Comment by Alex Miller [ 16/Jan/16 10:33 AM ]

The test may need to be made more strict, checking not just for sequential equality but also for a returned vector.

Just curious - was this issue causing a problem in your code or did you just notice it and find it surprising?

Comment by Eric Dvorsak [ 16/Jan/16 11:05 AM ]

Simple patch that just does for maps what is done for lists : Creates a new vector with the vec function.

Comment by Eric Dvorsak [ 16/Jan/16 11:08 AM ]

@Alex Miller : I noticed a bug in my program behavior and traced it down to a (get diff 2) instead of (nth diff 2), but I realized that it was only buggy in some cases so I looked further and found out if was coming from diff.

Comment by Eric Dvorsak [ 16/Jan/16 11:27 AM ]

More strict tests checking for a returned vector.





[CLJ-1881] Can :or destructuring refer to previous sequential bindings? Created: 11/Jan/16  Updated: 11/Jan/16

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

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

Approval: Triaged

 Description   

The following code works, but it is unspecified in the docs whether `(inc a)` can rely on `a` being bound.

user=> (defn foo [a {:keys [b] :or {b (inc a)}}]
  [a b])
user=> (foo 1 {:b 99})
[1 99] ;; :or not needed
user=> (foo 1 {})
[1 2]  ;; :or binds b to (inc a)

In sequential destructuring, are bindings bound in order such that subsequent :or value expressions can rely on prior sequential bindings?

This is true based on the current implementation of destructure, but looking for a statement to this effect in the docs and/or tests.






[CLJ-1880] IKVReduce impl for records Created: 09/Jan/16  Updated: 11/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord

Attachments: Text File CLJ-1880.patch    
Approval: Triaged

 Description   

Records don't implement IKVReduce, which could help with efficient merging (CLJ-1458)



 Comments   
Comment by Ghadi Shayban [ 11/Jan/16 2:49 PM ]

simple implementation attached





[CLJ-1875] Parameter names in docstring for `into` Created: 06/Jan/16  Updated: 20/Jan/16

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

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

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

 Description   

The docstring for into does not have the correct names for the parameters. The parameter names in the arglist are to, from and xform, but the doctring refers to them as to-coll from-coll, and the docstring does not refer to the optional transducer, xform by name.

Approach: update docstring to reflect param names
Patch: CLJ-1875.patch
Screened by:



 Comments   
Comment by Alex Miller [ 20/Jan/16 3:58 PM ]

The additional phrase at the end doesn't make sense to me. How about instead:

"A transducer, xform, may be supplied as an optional second argument."





[CLJ-1867] with-redefs used on a macro permanently changes it to a function Created: 10/Dec/15  Updated: 10/Dec/15

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

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

Approval: Triaged

 Description   

If you use with-redefs to redefine a macro (which is likely a mistake), the macro loses its macro status after the with-redefs call completes.

Presumably the fix depends on whether we think there is a valid use of with-redefs on a macro (which would only work if you're calling eval or equivalent in the body, and would require knowing enough about what you're doing to add the two extra macro args to your function) – if so, we would keep it from losing the macro status; if not, we might also have it throw an exception if you accidentally use it on a macro.

Demonstration of the effect:

user> (defmacro kwote [arg] `(quote ~arg))
#'user/kwote
user> (kwote hello)
hello
user> kwote
CompilerException java.lang.RuntimeException: Can't take value of a macro: #'user/kwote, compiling:(/tmp/form-init6222001939841513290.clj:1:18983)

;; Everything above is as expected

user> (with-redefs [kwote (constantly :in-with-redefs)] (kwote with-redefs-body))
with-redefs-body
user> (kwote hello)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: hello in this context, compiling:(/tmp/form-init6222001939841513290.clj:1:1) 
user> (kwote :arg-1)
ArityException Wrong number of args (1) passed to: user/kwote  clojure.lang.AFn.throwArity (AFn.java:429)
user> (kwote :arg-1 :arg-2 :arg-3)
(quote :arg-3)
user> kwote
#object[user$kwote 0x37e32ff6 "user$kwote@37e32ff6"]


 Comments   
Comment by Gary Fredericks [ 10/Dec/15 12:04 PM ]

Looks like the root cause is that with-redefs uses Var#bindRoot which intentionally clears the macro flag: https://github.com/clojure/clojure/blob/5cfe5111ccb5afec4f9c73b46bba29ecab6a5899/src/jvm/clojure/lang/Var.java#L270





[CLJ-1864] clojure.core/proxy does not work when reloading namespaces Created: 06/Dec/15  Updated: 08/Dec/15

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

Type: Defect Priority: Major
Reporter: Ralf Schmitt Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: protocols, proxy
Environment:

tested on 64 bit linux, oracle jdk 1.8


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

 Description   

clojure.core/proxy does not work when one reloads namespace containing defprotocol.

E.g. one can't reload the following file without triggering an error:

(ns foo.baz)

(defprotocol Hello
  (hello [this]))

(def hello-proxy
  (proxy [foo.baz.Hello] []
    (hello []
      (println "hello world"))))

(hello hello-proxy)

Saving the above as foo/baz.clj, I get the following error:

$ rlwrap java -cp target/clojure-1.8.0-master-SNAPSHOT.jar:. clojure.main
Clojure 1.8.0-master-SNAPSHOT
user=> (require 'foo.baz :reload)
hello world
nil
user=> (require 'foo.baz :reload)
CompilerException java.lang.IllegalArgumentException: No implementation of method: :hello of protocol: #'foo.baz/Hello found for class: foo.baz.proxy$java.lang.Object$Hello$6f95b989, compiling:(foo/baz.clj:11:1) 

I'm using the current git master (commit 5cfe5111ccb5afec4f9c73), but clojure 1.7 has the same problem.

The problem is that proxy-name only uses the interface names as a key. These names do not change when reloading the namespace, but the interfaces themself are new.

I'm going to attach a short patch which fixes that issue for me.



 Comments   
Comment by Ralf Schmitt [ 06/Dec/15 11:45 AM ]

I'm not sure how this interacts with AOT compilation.





[CLJ-1863] Bad type hints on a defn cause the compiler to throw a NPE Created: 04/Dec/15  Updated: 10/Jul/17

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

Type: Defect Priority: Major
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, errormsgs, typehints

Approval: Triaged

 Description   

After CLJ-1232 was committed to master, it is possible for the Clojure compiler to throw a NPE if a defn is type hinted with a invalid type. This surfaces in CLJS where the defn macro is re-used by the ClojureScript compiler, but I think it raises the question: "Should a bad type hint result in a compiler exception?"

The offending line can be found here on GitHub: https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L247



 Comments   
Comment by Alex Miller [ 18/Dec/15 8:12 AM ]

This is basically the same as CLJ-1868, but I think what you are asking here is whether bad type hints should be ignored or throw any exception, right?

(Whereas CLJ-1868 is about which exception/message is thrown)

Comment by Timothy Baldridge [ 18/Dec/15 8:22 AM ]

Agreed. I think another possible solution would be to update CLJS to not use the CLJ defn, but I still think that a bad type hint should just be ignored.

Comment by Nicola Mometto [ 18/Dec/15 8:29 AM ]

I don't agree that we shoud ignore bad type hints.
If the compiler knows that something is wrong, it should tell the user immediately rather than silently ignoring and potentially failing at runtime later

Comment by Alex Miller [ 10/Jul/17 2:09 PM ]

Description could use some examples





[CLJ-1852] Clojure-generated class names length exceed file-system limit Created: 20/Nov/15  Updated: 18/Oct/17

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

Type: Defect Priority: Major
Reporter: Martin Raison Assignee: Unassigned
Resolution: Unresolved Votes: 10
Labels: compiler
Environment:

tested on CentOS 6


Approval: Triaged

 Description   

Class names generated by the Clojure compiler can be arbitrarily long, exceeding the file system's maximum allowed file name length. For example it happens when you nest functions a bit too deeply:

(defmacro nestfn [n & body]
  (if (> n 0)
    `(fn [] (nestfn ~(- n 1) ~@body))
    body))

(def myf (nestfn 100 "body"))

Compiling this produces a java.io.IOException: File name too long exception.



 Comments   
Comment by Martin Raison [ 20/Nov/15 9:32 PM ]

The Scala community found this issue a while ago, and now the compiler has a max-classfile-name parameter (defaulting to 255). Hashing is used when the limit is exceeded. Maybe we should consider something similar?

Comment by Philipp Neumann [ 16/Oct/17 10:53 AM ]

I tried clojure.core.match with 13 patterns and the compiliation failed under Windows. I assume this problem is the root cause of it.





[CLJ-1843] Add =to function exposing Util/equivPred Created: 06/Nov/15  Updated: 15/May/17

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

Type: Feature Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-CLJ-1843-add-to-for-faster-equality-check-against-kn.patch    
Approval: Triaged

 Description   

Description:
It is sometimes useful to compare a collection of values against one value, clojure internally defines a predicate for this exact purpose which has some nice performance improvements over just a partial applied =.

Prior discussion with Rich: https://groups.google.com/forum/#!topic/clojure-dev/0c-VNhEKVkI

Example usage:

;;before:
(map (partial = 3) coll)
;;after:
(map (=to 3) coll)

Benchmarks:

test (partial = 'foo) #(= 'foo %) (=to 'foo)
small homogeneous coll 217ns 165ns 39ns
small eterogeneous coll, 192ns 167ns 41ns
big homogeneous coll 66us 52us 8us
big eterogeneous coll 82us 66us 27us

Full benchmarks output:

(use 'criterium.core)

(defn benchmark-f [f]
  (let [colls [['foo 'foo 'foo]
               [1 :foo 'foo]
               (doall (repeat 1e3 'foo))
               (doall (take 1e3 (cycle [1 :foo 'foo])))]]
    (doseq [c colls]
      (quick-bench (run! f c)))))

(benchmark-f (partial = 'foo))
ARNING: Final GC required 1.405293826432628 % of runtime
WARNING: Final GC required 10.202923149112559 % of runtime
Evaluation count : 3116130 in 6 samples of 519355 calls.
Execution time mean : 217.723199 ns
Execution time std-deviation : 29.425291 ns
Execution time lower quantile : 189.944710 ns ( 2.5%)
Execution time upper quantile : 261.717351 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 4.2579397621583315 % of runtime
Evaluation count : 3138636 in 6 samples of 523106 calls.
Execution time mean : 198.985418 ns
Execution time std-deviation : 12.691848 ns
Execution time lower quantile : 182.441245 ns ( 2.5%)
Execution time upper quantile : 207.839280 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 6.631646134523004 % of runtime
Evaluation count : 10038 in 6 samples of 1673 calls.
Execution time mean : 66.977712 µs
Execution time std-deviation : 10.411821 µs
Execution time lower quantile : 59.620690 µs ( 2.5%)
Execution time upper quantile : 84.483254 µs (97.5%)
Overhead used : 1.863362 ns

Found 1 outliers in 6 samples (16.6667 %)
low-severe  1 (16.6667 %)
Variance from outliers : 47.3059 % Variance is moderately inflated by outliers
WARNING: Final GC required 5.272721959665122 % of runtime
Evaluation count : 7908 in 6 samples of 1318 calls.
Execution time mean : 82.588512 µs
Execution time std-deviation : 5.215537 µs
Execution time lower quantile : 75.977936 µs ( 2.5%)
Execution time upper quantile : 86.849982 µs (97.5%)
Overhead used : 1.863362 ns


(benchmark-f #(= 'foo %))
WARNING: Final GC required 1.284421364203217 % of runtime
WARNING: Final GC required 10.04376144830405 % of runtime
Evaluation count : 3643032 in 6 samples of 607172 calls.
             Execution time mean : 165.393131 ns
    Execution time std-deviation : 1.041355 ns
   Execution time lower quantile : 164.277060 ns ( 2.5%)
   Execution time upper quantile : 166.849951 ns (97.5%)
                   Overhead used : 1.605524 ns
WARNING: Final GC required 6.258680973295933 % of runtime
Evaluation count : 3584574 in 6 samples of 597429 calls.
             Execution time mean : 167.659014 ns
    Execution time std-deviation : 3.821817 ns
   Execution time lower quantile : 164.175156 ns ( 2.5%)
   Execution time upper quantile : 173.210781 ns (97.5%)
                   Overhead used : 1.605524 ns

Found 1 outliers in 6 samples (16.6667 %)
	low-severe	 1 (16.6667 %)
 Variance from outliers : 13.8889 % Variance is moderately inflated by outliers
WARNING: Final GC required 6.914389197005716 % of runtime
Evaluation count : 11196 in 6 samples of 1866 calls.
             Execution time mean : 52.593759 µs
    Execution time std-deviation : 834.220092 ns



(benchmark-f (=to 'foo))
WARNING: Final GC required 7.40391654943877 % of runtime
Evaluation count : 15169068 in 6 samples of 2528178 calls.
Execution time mean : 39.937424 ns
Execution time std-deviation : 2.782661 ns
Execution time lower quantile : 37.393937 ns ( 2.5%)
Execution time upper quantile : 42.780432 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 5.986859953402835 % of runtime
Evaluation count : 15199992 in 6 samples of 2533332 calls.
Execution time mean : 41.229082 ns
Execution time std-deviation : 2.815533 ns
Execution time lower quantile : 37.371527 ns ( 2.5%)
Execution time upper quantile : 43.208673 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 5.039484046472016 % of runtime
Evaluation count : 69462 in 6 samples of 11577 calls.
Execution time mean : 8.976972 µs
Execution time std-deviation : 587.089991 ns
Execution time lower quantile : 8.505317 µs ( 2.5%)
Execution time upper quantile : 9.744296 µs (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 5.773010947849351 % of runtime
Evaluation count : 23352 in 6 samples of 3892 calls.
Execution time mean : 27.277376 µs
Execution time std-deviation : 2.115666 µs
Execution time lower quantile : 25.719322 µs ( 2.5%)
Execution time upper quantile : 30.123547 µs (97.5%)
Overhead used : 1.863362 ns

Patch: 0001-CLJ-1843-add-to-for-faster-equality-check-against-kn.patch



 Comments   
Comment by Alex Miller [ 06/Nov/15 9:15 AM ]

Did you look at (apply = 3 coll) ? Just curious.

Comment by Nicola Mometto [ 06/Nov/15 9:19 AM ]

The advantage of Util/equivPred over Util/equiv is that it can assume the type of the provided argument, avoiding the runtime cost of doing the multiple instance checks that Util/equiv has to do to figure out what comparator to use internally

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

Could you quantify the difference between these approaches on 2-3 collection sizes?

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

With the following setup:

(use 'criterium.core)

(defn =to [x]
  (let [pred (clojure.lang.Util/equivPred x)]
    (fn [y]
      (.equiv pred x y))))

(defn benchmark-f [f]
  (let [colls [['foo 'foo 'foo]
               [1 :foo 'foo]
               (doall (repeat 1e3 'foo))
               (doall (take 1e3 (cycle [1 :foo 'foo])))]]
    (doseq [c colls]
      (quick-bench (run! f c)))))

The results for (benchmark-f (partial = 'foo) are:

WARNING: Final GC required 1.405293826432628 % of runtime
WARNING: Final GC required 10.202923149112559 % of runtime
Evaluation count : 3116130 in 6 samples of 519355 calls.
Execution time mean : 217.723199 ns
Execution time std-deviation : 29.425291 ns
Execution time lower quantile : 189.944710 ns ( 2.5%)
Execution time upper quantile : 261.717351 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 4.2579397621583315 % of runtime
Evaluation count : 3138636 in 6 samples of 523106 calls.
Execution time mean : 198.985418 ns
Execution time std-deviation : 12.691848 ns
Execution time lower quantile : 182.441245 ns ( 2.5%)
Execution time upper quantile : 207.839280 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 6.631646134523004 % of runtime
Evaluation count : 10038 in 6 samples of 1673 calls.
Execution time mean : 66.977712 µs
Execution time std-deviation : 10.411821 µs
Execution time lower quantile : 59.620690 µs ( 2.5%)
Execution time upper quantile : 84.483254 µs (97.5%)
Overhead used : 1.863362 ns

Found 1 outliers in 6 samples (16.6667 %)
low-severe  1 (16.6667 %)
Variance from outliers : 47.3059 % Variance is moderately inflated by outliers
WARNING: Final GC required 5.272721959665122 % of runtime
Evaluation count : 7908 in 6 samples of 1318 calls.
Execution time mean : 82.588512 µs
Execution time std-deviation : 5.215537 µs
Execution time lower quantile : 75.977936 µs ( 2.5%)
Execution time upper quantile : 86.849982 µs (97.5%)
Overhead used : 1.863362 ns

The results fore (benchmark-f (=to 'foo)) are:

WARNING: Final GC required 7.40391654943877 % of runtime
Evaluation count : 15169068 in 6 samples of 2528178 calls.
Execution time mean : 39.937424 ns
Execution time std-deviation : 2.782661 ns
Execution time lower quantile : 37.393937 ns ( 2.5%)
Execution time upper quantile : 42.780432 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 5.986859953402835 % of runtime
Evaluation count : 15199992 in 6 samples of 2533332 calls.
Execution time mean : 41.229082 ns
Execution time std-deviation : 2.815533 ns
Execution time lower quantile : 37.371527 ns ( 2.5%)
Execution time upper quantile : 43.208673 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 5.039484046472016 % of runtime
Evaluation count : 69462 in 6 samples of 11577 calls.
Execution time mean : 8.976972 µs
Execution time std-deviation : 587.089991 ns
Execution time lower quantile : 8.505317 µs ( 2.5%)
Execution time upper quantile : 9.744296 µs (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 5.773010947849351 % of runtime
Evaluation count : 23352 in 6 samples of 3892 calls.
Execution time mean : 27.277376 µs
Execution time std-deviation : 2.115666 µs
Execution time lower quantile : 25.719322 µs ( 2.5%)
Execution time upper quantile : 30.123547 µs (97.5%)
Overhead used : 1.863362 ns
Comment by Nicola Mometto [ 06/Nov/15 3:07 PM ]

Using #(= 'foo %) rather than (partial = 'foo) allows for inlining of = and makes performance a bit better, but =to still wins noticeably

WARNING: Final GC required 1.284421364203217 % of runtime
WARNING: Final GC required 10.04376144830405 % of runtime
Evaluation count : 3643032 in 6 samples of 607172 calls.
             Execution time mean : 165.393131 ns
    Execution time std-deviation : 1.041355 ns
   Execution time lower quantile : 164.277060 ns ( 2.5%)
   Execution time upper quantile : 166.849951 ns (97.5%)
                   Overhead used : 1.605524 ns
WARNING: Final GC required 6.258680973295933 % of runtime
Evaluation count : 3584574 in 6 samples of 597429 calls.
             Execution time mean : 167.659014 ns
    Execution time std-deviation : 3.821817 ns
   Execution time lower quantile : 164.175156 ns ( 2.5%)
   Execution time upper quantile : 173.210781 ns (97.5%)
                   Overhead used : 1.605524 ns

Found 1 outliers in 6 samples (16.6667 %)
	low-severe	 1 (16.6667 %)
 Variance from outliers : 13.8889 % Variance is moderately inflated by outliers
WARNING: Final GC required 6.914389197005716 % of runtime
Evaluation count : 11196 in 6 samples of 1866 calls.
             Execution time mean : 52.593759 µs
    Execution time std-deviation : 834.220092 ns
   Execution time lower quantile : 51.510161 µs ( 2.5%)
   Execution time upper quantile : 53.367649 µs (97.5%)
                   Overhead used : 1.605524 ns
WARNING: Final GC required 6.179040224498723 % of runtime
Evaluation count : 9162 in 6 samples of 1527 calls.
             Execution time mean : 66.527357 µs
    Execution time std-deviation : 2.119652 µs
   Execution time lower quantile : 65.308835 µs ( 2.5%)
   Execution time upper quantile : 70.201570 µs (97.5%)
                   Overhead used : 1.605524 ns

small homogeneous coll, (partial = 'foo): 217ns, #(= 'foo %): 165ns, (=to 'foo): 39ns
small eterogeneous coll, (partial = 'foo): 192ns, #(= 'foo %): 167ns, (=to 'foo): 41ns
big homogeneous coll, (partial = 'foo): 66us, #(= 'foo %): 52us, (=to 'foo): 8us
big eterogeneous coll, (partial = 'foo: 82us, #(= 'foo %): 66us, (=to 'foo): 27us

Comment by Nicola Mometto [ 17/Dec/15 5:13 PM ]

Apparently this was something that was already discussed a couple of years ago and Rich seemed ok with this https://groups.google.com/forum/#!topic/clojure-dev/0c-VNhEKVkI





[CLJ-1836] Expose clojure.repl/doc as a function call Created: 28/Oct/15  Updated: 15/May/17

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

Type: Feature Priority: Minor
Reporter: Terje Dahl Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: repl

Attachments: Text File doc-fn-1.patch    
Approval: Triaged

 Description   

Restructure the printing function clojure.repl/doc so that it calls a fuction clojure.repl/doc-fn for its data - in the same way as dir calls dir-fn. Make doc-fn public so that it can be called directly and allow developers to parse and display the data as needed.

Use case: I am making a namespace inspector (using JavaFX) (somewhat like the Swing-based tree-inspector in Clojure), and when getting a function, I would like to display the same meta-information as "doc" prints in the REPL - including the special forms data coded in a private var/map in Clojure.

Patch: doc-fn.patch



 Comments   
Comment by Alex Miller [ 28/Oct/15 12:34 PM ]

A few comments:

1) Patch authors must sign the contributor's agreement, see http://clojure.org/contributing

2) The patch is not in the correct format - see http://dev.clojure.org/display/community/Developing+Patches for more info.

3) Patch should include a test for the new doc-fn.

Comment by Terje Dahl [ 21/Nov/15 6:29 AM ]

1. Agreement signed.

2. Tests added.
(That was useful! I had to fix a couple of things.)

3. Patch created as per instructions and attached:
"doc-fn-1.patch"





[CLJ-1818] cl-format does not respect aesthetic ~A with a keyword Created: 26/Sep/15  Updated: 12/Jan/16

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

Type: Defect Priority: Trivial
Reporter: Jong-won Choi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print

Approval: Triaged

 Description   

In Common Lisp, (format nil "~a" :A) returns "A". However, in Clojure, it returns with the colon:

(clojure.pprint/cl-format false "~a" :A)
=> ":A"


 Comments   
Comment by Jong-won Choi [ 28/Sep/15 6:26 AM ]

Found another problem of cl-format:

(clojure.pprint/cl-format false "SELECT * from RateSchedules ~@[WHERE ~{~A=?~^ ~}~]" '())
=> "SELECT * from RateSchedules WHERE" ;; instead of "SELECT * from RateSchedules"

I guess the problem is () or [] has to be treated as falsey but not.

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

:a is a keyword and I would expect it's ascii format to be :a. I'm not sure what case sensitivity has to do with it.

Comment by Andy Fingerhut [ 28/Sep/15 10:08 AM ]

Alex, case is a side issue. Common Lisp's (format nil "~a" :A) returns "A", not ":A". It is the presence of the colon in the output that is the issue, not the case of the string.

Comment by Jong-won Choi [ 28/Sep/15 4:41 PM ]

For a record, what Alex described is for ~S - standard. See http://www.lispworks.com/documentation/lw50/CLHS/Body/22_cd.htm





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

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

Type: Feature Priority: Major
Reporter: Tristan Strange Assignee: Colin Taylor
Resolution: Unresolved Votes: 5
Labels: error-reporting
Environment:

All Clojure platforms


Attachments: Text File CLJ-1817.patch    
Approval: Triaged

 Description   

A failing in a predicate in a list of :pre or :post conditions currently causes messages similar to one below to be displayed:

(defn must-be-a-map [m] {:pre [(map? m)]} m)
(must-be-a-map [])
;;=> AssertionError Assert failed: (map? m)  user/must-be-a-map (form-init.....clj:1)

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

Predicate functions and there associated messages strings could be specified as a pair of values in a map:

(defn must-be-a-map 
  [m]
  {:pre [{(map? m) "m must be a map due to some domain specific reason."}]}
  m)

The following would then produce an error message as follows:

(must-be-a-map 10)
AssertionError Assert failed: m must be a map due to some domain specific reason.
(map? m) user/must-be-a-map (form-init.....clj:1)

This would allow predicates without messages to specified alongside pairs of associated predicate message pairs as follows:

(defn n-and-m [n m] {:pre [(number? n) {(map? m) "You must provide a map!"}]})

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

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

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



 Comments   
Comment by Colin Taylor [ 03/Apr/16 5:26 PM ]

Attached approach differs from that advocated for in the description by not requiring a map. The existing spec of :

{:pre [pre-expr*]
 :post [post-expr*]}

in effect becoming :

{:pre [(pre-expr assert-msg?)*]
 :post [(pre-expr assert-msg?)*]}

where assert-msg is a String. Note this means a (presumably erroneous) second String after an expression would be treated as a truthy pre-expr.

Contrived example :

(defn print-if-alphas-and-nums [arg] {:pre [(hasAlpha arg) "No alphas"
                                            (hasNum arg) "No numbers"
                                            (canPrint arg)]}
  (println arg))

user=> (print-if-alphas-and-nums "a5%")
a5%
nil
user=> (print-if-alphas-and-nums "$$%")
AssertionError Assert failed: No alphas
(hasAlpha arg)  user/print-if-alphas-and-nums (NO_SOURCE_FILE:19)

I have considered extending the spec further to (pre-expr assert-msg? data-map)* perhaps supported by ex-info, ex-data analogues in assert-info, assert-data to convey diagnostic info (locals?). A map could contain a :msg key or perhaps the map is additional to the message string. I thought I'd wait for input though at this point.

I also considered allowing % substitution for the fn return value in the message as in :post conds, but how to escape?

Comment by Colin Taylor [ 03/Apr/16 6:17 PM ]

I should point out that the tests include the currently uncovered existing functionality too.





[CLJ-1811] test line reporting doesn't always report test's file & line number Created: 02/Sep/15  Updated: 03/Dec/15

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

Type: Enhancement Priority: Minor
Reporter: Ryan Fowler Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: test

Attachments: Text File clj-1811.patch     File error_reporting.clj     Text File LINE_REPORING_1.patch     Text File LINE_REPORING_2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

If an Exception is thrown during test execution, the filename and line number are frequently not helpful for finding the problem. For instance, this code:

error_reporting.clj
(require '[clojure.test :refer [deftest test-var]])

(deftest foo
  (meta))

(test-var #'foo)

Will output an error at AFn.java:429.

ERROR in (foo) (AFn.java:429)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ArityException: Wrong number of args (0) passed to: core/meta--4144
 at clojure.lang.AFn.throwArity (AFn.java:429)
    clojure.lang.AFn.invoke (AFn.java:28)
    user/fn (error_reporting.clj:4)
    clojure.test$test_var$fn__7670.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)
    user$eval6.invoke (error_reporting.clj:6)
    clojure.lang.Compiler.eval (Compiler.java:6782)
    ...etc

Rich's Comment 24016 on CLJ-377 says that he thinks the message should report the test file line rather than where the exception was thrown.

Approach: Filter the stacktrace class prefix clojure.lang.AFn from the top of error stacktraces.

After applying the patch, the above example outputs error_reporting.clj:4:

ERROR in (foo) (error_reporting.clj:4)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ArityException: Wrong number of args (0) passed to: core/meta--4141
 at clojure.lang.AFn.throwArity (AFn.java:429)
    clojure.lang.AFn.invoke (AFn.java:28)
    user$fn__3.invokeStatic (error_reporting.clj:4)
    user/fn (error_reporting.clj:3)
    clojure.test$test_var$fn__114.invoke (test.clj:705)
    clojure.test$test_var.invokeStatic (test.clj:705)
    clojure.test$test_var.invoke (test.clj:-1)
    user$eval6.invokeStatic (error_reporting.clj:6)
    user$eval6.invoke (error_reporting.clj:-1)
    clojure.lang.Compiler.eval (Compiler.java:6939)
    ...etc

Patch: clj-1811.patch



 Comments   
Comment by Ryan Fowler [ 02/Sep/15 5:36 PM ]

example file from description

Comment by Alex Miller [ 04/Sep/15 10:04 AM ]

A quick search on Github shows many cases where people call into the (admittedly private) file-and-line function. These users would be broken by the patch. Perhaps it would be better to create a new function or a new arity rather than removing the existing arity.

Just eyeballing it, but I suspect you've introduced reflection in a couple places in the new code, particularly these might need another type hint:

1. (.getName (.getClass (:test (meta test-var))))
2. #(= (.getClassName %) test-var-class-name)

I need to look at more of the code to make a judgement on everything else. Seeing testing-vars in there means that this function is now dependent on external state, so need to think carefully to be sure that every calling context has that global state (or won't fail in bad ways if it doesn't). It would be helpful to see a discussion of your thinking about that in the "approach" section of the ticket description.

Comment by Ryan Fowler [ 30/Sep/15 3:41 PM ]

Second attempt at a patch to resolve this issue. Corrects issues Alex pointed out

Comment by Ryan Fowler [ 30/Sep/15 3:53 PM ]

I've filled in some detail in the approach section.

I also added a new patch LINE_REPORTING_2.patch that addresses reflection warnings, restores the old arity of file-and-line and adds protection from people calling file-and-line from outside a testing context.

Comment by Ryan Fowler [ 30/Sep/15 6:20 PM ]

While discussing an issue with my coworker James, I realized that this fix helps with shared functions calling (is). Notice how the run of this sample code reports line 7 with LINE_REPORTING_2.patch applied. This test line is generally much more useful than the shared function line.

example_2
ryans-mbp:~/oss/clojure% cat -n error_reporting.clj
     1  (require '[clojure.test :refer [deftest test-var is]])
     2
     3  (defn shared-code [arg]
     4    (is arg))
     5
     6  (deftest test-shared-code
     7    (shared-code false))
     8
     9  (test-var #'test-shared-code)
ryans-mbp:~/oss/clojure% java -jar ~/.m2/repository/org/clojure/clojure/1.7.0/clojure-1.7.0.jar ./error_reporting.clj

FAIL in (test-shared-code) (error_reporting.clj:4)
expected: arg
  actual: false
ryans-mbp:~/oss/clojure% java -jar target/clojure-1.8.0-master-SNAPSHOT.jar ./error_reporting.clj

FAIL in (test-shared-code) (error_reporting.clj:7)
expected: arg
  actual: false
Comment by Michael Blume [ 02/Dec/15 5:55 PM ]

Patch doesn't apply anymore, but maybe this has been sorted by CLJ-1856?

Comment by Alex Miller [ 03/Dec/15 9:57 AM ]

This is not fixed by CLJ-1856, but could be if clojure.lang.AFn was filtered out of error stacktraces when finding the location. This is pretty specific - it might make sense to broaden to other calling paths too, not sure.

Attached a new patch that applies this filtering.





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

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

Type: Feature Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: protocols

Attachments: Text File 0001-CLJ-1807-add-prefer-proto.patch    
Approval: Triaged

 Description   

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

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

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

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

Example:

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

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






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

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

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

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

 Description   

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

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

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


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

Proposed patch, passes all existing tests

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

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

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

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

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

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

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

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

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

(now ~35% faster than original)





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

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

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

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

 Description   

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

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

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

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

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

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

So using such a thing,

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

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

The attached patch makes this change.



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

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

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

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

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

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

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

1.8.0-master:

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

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

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

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

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





[CLJ-1800] Doc that lazy-seq with-meta forces realization Created: 13/Aug/15  Updated: 19/Aug/15

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

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

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

 Description   

Applying meta to a lazy-seq causes realization:

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

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

Patch:



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

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

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

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

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

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

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

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

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

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

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

Added test





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

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

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

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

Mac OSX 10.10.4


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

 Description   

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

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



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

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

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

I found it uses many dosync in writer as below:

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

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





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

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

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

Approval: Triaged

 Description   
(defprotocol TestProtocol
  (tester [o]))

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

(def another-tester2 tester)

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

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

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

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

(def another-tester2 tester)

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

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

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


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

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

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

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

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

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

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

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

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





[CLJ-1794] Sorting vector yields non-indexed ArraySeq Created: 05/Aug/15  Updated: 28/Sep/17

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

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

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

 Description   

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

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

Alternatives:

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



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

Update description, attach patch.

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

Added link to current patch.

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

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

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

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

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

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

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

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





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

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

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

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

 Description   

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

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

(reset-test)

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

Expected behavior

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

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

(swap-test)

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

Alternative expected behavior

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

Analysis

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

Approach

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

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


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

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

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

Results

Existing implementation:

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

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

Suggested implementation:

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

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

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

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





[CLJ-1768] quote of an empty lazyseq produces an error when evaled Created: 24/Jun/15  Updated: 26/Apr/16

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

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

Approval: Triaged

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

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



 Comments   
Comment by Tim Engler [ 26/Apr/16 4:17 AM ]

Still exists in clojure 1.8





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

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

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

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

 Description   

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

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

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

Patch: 0001-