<< Back to previous view

[CLJ-2210] back referential expressions can cause exponential compilation times Created: 21/Jul/17  Updated: 24/Jul/17

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: compiler, performance

Attachments: Text File 0001-CLJ-2210-cache-non-trivial-getJavaClass-hasJavaClass.patch    
Patch: Code
Approval: Triaged

 Description   

Reported as causing problems in real world code: https://groups.google.com/forum/#!topic/clojure/Z91bhUvSB1g

With init.clj as :

(def expr '(fn [& {:keys [a b c d e f
                          map-0 map-1 map-2]}]
             (cond-> "foo"
               a (str a)
               b (str b)
               c (str c)
               d (str d)
               e (str e)
               f (str f)
               map-0 (cond->
                         (:a map-0) (str (:a map-0))
                         (:b map-0) (str (:b map-0))
                         (:c map-0) (str (:c map-0))
                         (:d map-0) (str (:d map-0)))
               map-1 (cond->
                         (:a map-1) (str (:a map-1))
                         (:b map-1) (str (:b map-1))
                         (:c map-1) (str (:c map-1))
                         (:d map-1) (str (:d map-1)))
               map-2 (cond->
                         (:a map-2) (str (:a map-2))
                         (:b map-2) (str (:b map-2))
                         (:c map-2) (str (:c map-2))
                         (:d map-2) (str (:d map-2))))))

(time (eval expr))

Before patch:

[~]> clj -i init.clj
"Elapsed time: 24233.193238 msecs"

After patch:

[~]> clj -i init.clj
"Elapsed time: 24.719472 msecs"

This is caused by every local bindings' type depending on the type of the previous binding, triggering an exponential number of calls to hasJavaClass and getJavaClass. By caching on first occurrence, the complexity becomes linear

Patch: 0001-CLJ-2210-cache-non-trivial-getJavaClass-hasJavaClass.patch






[CLJ-2209] getting started documentation for 1.9.0-alpha uses 1.8.0.jar and does not work with 1.9.0-alpha17.jar due to missing specs related jars in the classpath Created: 20/Jul/17  Updated: 20/Jul/17  Resolved: 20/Jul/17

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

Type: Defect Priority: Major
Reporter: Michael A Wright Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

clojure-1.9.0-alpha17
Java 1.8.0_73-b02
Mac OS X 10.9.5



 Description   

in Https://clojure.org/guides/getting_started when I try:
java -cp clojure-1.8.0.jar clojure.main
it works (starts a REPL) as expected.
When I change to clojure-1.9.0-alpha17.jar I get an error
Caused by: java.io.FileNotFoundException: Could not locate clojure/spec/alpha__init.class or clojure/spec/alpha.clj on classpath.

In order to start a REPL this way with 1.9.0-alpha17, I had to use lein to fetch other dependencies and construct a command like this:

java -cp $HOME/.m2/repository/org/clojure/clojure/1.9.0-alpha17/clojure-1.9.0-alpha17.jar:$HOME/.m2/repository/org/clojure/spec.alpha/0.1.123/spec.alpha-0.1.123.jar:$HOME/.m2/repository/org/clojure/core.specs.alpha/0.1.10/core.specs.alpha-0.1.10.jar:. clojure.main

I also was able to unjar the three jar files and rejar into one jar file to simplify the classpath to one jar.

I'd like to see the getting started documentation address this somehow for Clojure newcomers (after 1.9.0 is released) or for experienced Clojure users that want a quicker way to start a REPL (instead of boot or lein) occasionally.

Perhaps there could be a way to bootstrap a minimal clojure repl without using lein or boot (and without requiring the downloading, and specifying the classpath for, three jar files? It would be nice for newcomers to kick the tires with minimal effort. Perhaps a simplified version of clojure.jar called clojure-repl.jar?



 Comments   
Comment by Michael A Wright [ 20/Jul/17 2:55 PM ]

https://groups.google.com/forum/#!msg/clojure/10dbF7w2IQo/ec37TzP5AQAJ has some discussion related to this. Is there another issue or plan somewhere that is tracking the work needed to update the Getting Started section to correctly work with a clojure-1.9.0.jar when it is release?

Comment by Alex Miller [ 20/Jul/17 9:40 PM ]

Hi Michael, I actually gave a talk about our plans for 1.9 in this area today at EuroClojure and I will have a better writeup for public consumption on that next week.

The Getting Started page is (always) targeted at the current stable release, currently 1.8, and that's something that we will update when Clojure 1.9 is released as part of our typical release process. In this case, it will be substantially updated based on the new stuff coming shortly. Because this is part of our standard release process, I don't need a ticket for it here so I will close this.





[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-2207] Clojure reader should treat non-breaking space as whitespace character Created: 14/Jul/17  Updated: 16/Jul/17

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

Type: Enhancement Priority: Minor
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader

Attachments: Text File clj-2207-nbsp.patch     Text File clj-2207-nbsp-v2.patch     Text File clj-2207-nbsp-v3.patch    
Patch: Code
Approval: Prescreened

 Description   

Right now, Clojure uses Character.isWhitespace(ch) || ch == ',' as the definition of whitespace in the reader. Character.isWhitespace, however, for obscure reasons (it has been defined long time ago), intentionally excludes U+00A0 (no-break space), U+2007 (figure space), U+202F (narrow no-break space). Logically, though, these characters should be treated as normal whitespace for all reasons except text formatting (e.g. the newer Character.isSpaceChar fixed that and does treat them as space chars).

Why is this important: if non-breaking space is inserted by accident (e.g. by pressing Option+Space on Mac), it'll be very hard to find the source of the error in a otherwise very innocent-looking code.

The attached patch implements Util.isWhitespace method that returns true for all characters treated as whitespace by Character.isWhitespace AND for those 3 exceptions. All cases where reading used Character.isWhitespace was referenced are modified to call new Util.isWhitespace instead.

Patch: clj-2207-nbsp-v3.patch

Prescreened by: Alex Miller



 Comments   
Comment by Alex Miller [ 14/Jul/17 10:12 AM ]

I think most of the locations that you changed are not relevant to your request and complicate the evaluation of this patch. Including support for these characters as whitespace in the reader seems fine to me. Determining whether these should be changed in formatting, string, xml, etc function requires more analysis and may not be worth doing so you should narrow this patch to just the changes in LispReader and EdnReader.

Also, the isWhitespace() method should not be in RT (which triggers loading of the Clojure runtime) and would be better in the Util class.

Comment by Nikita Prokopov [ 14/Jul/17 12:46 PM ]

Thanks Alex! Very valuable comments, thank you. See new patch attached. I took a freedom to alter main.clj as well because I feel like it should stay in sync with what ListReader/EdnReader do.

Comment by Alex Miller [ 14/Jul/17 1:44 PM ]

Why did you add the isWhitespace() method in LispReader too? Just make one method in Util and use it from everywhere.

Comment by Nikita Prokopov [ 14/Jul/17 5:52 PM ]

As you said, I didn’t want to change too much. But yes, if we’re not using it in string/xml/formatter it probably can be extracted. See new patch attached.

Comment by Phill Wolf [ 16/Jul/17 7:53 AM ]

javac does not accept non-breaking spaces. The message is

nonbreakingspace.java:4: error: illegal character: '\u00a0'

Do we really want a codebase with an uncontrolled variety of spaces and non-breaking spaces?

If non-breaking spaces are inadvertent, would an editor macro be more appropriate, or a Git hook, or a more pointed Clojure error message?

I am still smarting from tabs-vs-spaces. Java's \s does not match non-breaking spaces. I will be disappointed if things start hiding themselves from grep by using exotic spaces.

Overall - it seems to me that enlarging the category of invisible alternatives for whitespace in source code may not lead to a net improvement in quality of life.





[CLJ-2206] Facilities to alias (re-import) functions, variables and macros Created: 13/Jul/17  Updated: 13/Jul/17  Resolved: 13/Jul/17

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

Type: Feature Priority: Minor
Reporter: Vitalie Spinu Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Organizing namespaces can be hard. Especially in Clojurescript where, due two stage macro compilation, creation of artificial namespaces leaves little room to move around. An often used re-def (def xyz ns/xyz) doesn't preserve metadata.

Zach Tellman's potemkin implements import facilities for functions, vars and macros in Clojure. Would be great to have something similar in Clojure(script) proper.



 Comments   
Comment by Alex Miller [ 13/Jul/17 2:04 PM ]

Thanks, but we do not expect to add var import functionality to Clojure. Namespaces exist to give context to vars and allowing them to be "imported" to other namespaces works against the use of namespaces for naming.

If ClojureScript has problems specific to ClojureScript, solutions can be considered there as needed.





[CLJ-2205] Consider security implications of proxy implementation Created: 12/Jul/17  Updated: 17/Jul/17  Resolved: 17/Jul/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2, Release 1.3, Release 1.4, Release 1.5, Release 1.6, Release 1.7, Release 1.8
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Chouser
Resolution: Duplicate Votes: 1
Labels: proxy, security


 Description   

Per the explanation in CLJ-2204, AOT compiled proxies of Serializable/Externalizable classes are susceptible to misuse for deserialization attacks. We should consider modifications to proxy to detect and warn or prevent this kind of misuse.



 Comments   
Comment by Chouser [ 17/Jul/17 11:06 PM ]

Superceded by CLJ-2204





[CLJ-2204] Clojure classes can be used to craft a serialized object that runs arbitrary code on deserialization Created: 12/Jul/17  Updated: 18/Jul/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2, Release 1.3, Release 1.4, Release 1.5, Release 1.6, Release 1.7, Release 1.8
Fix Version/s: Release 1.9

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: inspector, security

Attachments: Text File clj-2204-disable-proxy-serialization.patch     Text File clj-2204.patch    
Patch: Code and Test
Approval: Vetted

 Description   

If a server can deserialize objects from an untrusted source, it is possible to craft a serialized object that runs arbitrary code on deserialization. Classes in the Clojure jar can be used for this purpose and this may lead to blacklisting the Clojure jar in some environments.

Demonstration of how to create such an object: https://github.com/frohoff/ysoserial/pull/68/files

The serialized class being constructed in the attack is clojure.inspector.proxy$javax.swing.table.AbstractTableModel$ff19274a, but clojure.core.proxy$clojure.lang.APersistentMap$ff19274a and proxy classes created by users are also susceptible. Clojure proxies contain a field (__clojureFnMap) that is a map of method names to proxy functions. Clojure AOT compiles the clojure.inspector and clojure.core.proxy code which generates the classes named above. These classes are then included inside the clojure jar.

The attack constructs an instance of one of these proxy classes and adds a "hashCode" proxy method to the proxy's table. The method is a Clojure function that can run arbitrary code. This instance is then put inside a HashMap and the whole thing is serialized. On deserialization, the HashMap will invoke hashCode() on the proxy object and cause the execution of the arbitrary code.

Note that most uses of proxy will create objects that cannot be serialized anyway, due to the common use of literal function bodies.

Proposed: Modify proxy so that the classes it generates are neither deserializable (to disable the attack) nor serializable (to avoid misleading users). The patch causes proxy classes to have readObject and writeObject methods that just throw an exception, when Serializable is in the inheritance tree of the proxy class. This is technically a breaking change since it is possible to construct a proxy that can be serialized, but such classes are unlikely in the wild and inherently insecure.

We should consider releasing Clojure 1.8.1 with this as well.

Patch: clj-2204-disable-proxy-serialization.patch

Background: Raised at https://groups.google.com/d/msg/clojure/WaL3hHzsevI/7zHU-L7LBQAJ and similar to issues reported on Apache Commons. See also:



 Comments   
Comment by Chouser [ 16/Jul/17 5:47 PM ]

I believe there are other proxies of a Serializable classes. I wrote a little script (included below) to dig through a target directory of AOT classes looking for suspicious ones. It may generate false positives, but one it found is generated from this line, and APersistentMap does inherit from java.io.Serializable. Using this clue, I was then able to change the gadget code to use clojure.core.proxy$clojure.lang.APersistentMap$ff19274a instead of the AbstractTableModel proxy and got equivalent results.

I'll keep looking for other classes, but I think we'll unfortunately need to do more than just skip AOT'ing the inspector namespace.

#!/bin/bash

java -cp $HOME/.m2/repository/org/clojure/spec.alpha/0.1.123/spec.alpha-0.1.123.jar:$HOME/.m2/repository/org/clojure/core.specs.alpha/0.1.10/core.specs.alpha-0.1.10.jar:target/classes clojure.main - <<"EOF"

(ns cc
  (:require [clojure.java.io :as io]
            [clojure.reflect :as reflect]
            [clojure.string :as str]
            [clojure.pprint :refer [pprint]]))

(defn ancestor-strs [classname]
 (set (map str (:ancestors (reflect/type-reflect (Class/forName classname) :ancestors true)))))

(->> (file-seq (io/file "target/classes")) ;; all files and dirs
 (keep #(second (re-matches #"target/classes/(.*)\.class" (str %)))) ;; class filenames
 (map #(str/replace % #"/" ".")) ;; classnames
 (filter #(let [as (conj (ancestor-strs %) %)]
	   (and
	    (contains? as "java.io.Serializable")
	    (some (fn [a] (re-find #"proxy" a)) as))))
 prn)

EOF
Comment by Chouser [ 16/Jul/17 9:22 PM ]

Here is the gadget code plus in-memory test harness written in Clojure; makes it a bit easier to test proxy classes for this issue:

(def flag (atom true))                                                         
                                                                               
(defn ok-proxy? [target-proxy]                                                 
  (let [baos (java.io.ByteArrayOutputStream.)                                  
        top (java.util.HashMap.                                                
              {(doto target-proxy                                              
                 (.__initClojureFnMappings {"hashCode" (constantly 0)})) 0})]  
    (.__initClojureFnMappings                                                  
      target-proxy                                                             
      {"hashCode" (comp eval (constantly '(do (reset! flag false) 0)))})       
    (.writeObject (java.io.ObjectOutputStream. baos) top)                      
    (let [payload (.toByteArray baos)]                                         
      (reset! flag true)                                                       
                                                                               
      ;; Deserializing the payload will execute hashCode method above          
      (-> payload                                                              
        java.io.ByteArrayInputStream. java.io.ObjectInputStream. .readObject)  
      @flag)))                                                                 
                                                                               
(prn :ok? (ok-proxy? (bean (Object.))))                                        
(prn :ok? (ok-proxy? (inspector/list-model nil)))                              
Comment by Chouser [ 16/Jul/17 10:03 PM ]

I think we should change the proxy code to generate classes that explicitly refuse to support deserialization. Proxy objects already generally fail to serialize due to the function objects in their __clojureFnMap, so this would not be removing any useful functionality that I'm aware of. I've got a proof of concept here in which proxy always supplies private readObject and writeObject methods, complaining usefully instead of writing, and absolutely refusing to read. Does this seem like a good approach?

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

Hey Chouser, thanks for working on this and finding my gaps! Making proxies nonserializable seems like a good approach to me. Should also cover writeExternal() and readExternal() for Externalizable I think?

Comment by Chouser [ 17/Jul/17 11:22 AM ]

Thanks, as always, for your diligence and expertise on so many of these issues.

It's not clear to me if we need to do the Externalizable methods (I'm only just now learning about these serialization features). Even if a proxied class does implement Externalizable, it would have to provide methods that explicitly serialize and deserialize the proxy class's FnMap – which of course nothing in Java core can do. If an application were to do this, it would not really be Clojure's responsibility to prevent it, even if it could, I think. Am I getting this right?

If we do implement Externailzable methods to prevent them from doing anything dangerous, it would only be for classes that inherit from Externalizable. This is in contrast to readObject/writeObject which are not methods of any interface, and so I believe are essentially harmless to provide at all times.

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

Yep, that makes sense. Externalizable classes would have explicitly made the choice to implement these methods and would not be serializing state of proxy subclass.

I suppose the only potential harm in providing readObject() and writeObject() would be in the (unlikely) case where the parent class was not Serializable but had those methods. I guess we could narrow to just providing them if the class implemented Serializable.

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

Also, I filed a separate ticket (CLJ-2205) re the broader proxy serialization issue which seems like where this is headed. It would probably make sense to rewrite the description here to cover the more general fix and to close CLJ-2205 as a duplicate.

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

Patch looks good to me - can you add a test with the harness to the patch too?

Comment by Chouser [ 17/Jul/17 11:15 PM ]

Thanks for reviewing it.

I will add a test; it could simply confirm that an attempt to serialize a proxy class fails. Do you think that's sufficient, or should it also include the attempted code execution? I guess the latter could fail for other more subtle reasons, causing the test to pass when perhaps we wouldn't want it to, so I'll go for the simpler test first.

Comment by Alex Miller [ 17/Jul/17 11:22 PM ]

I think the simpler test is sufficient.

Comment by Chouser [ 18/Jul/17 11:00 AM ]

Attached patch with fix and test. Note the test for prohibited deserialization is tricky because we also prevent the creation of the data needed for that test. Because of this, the serialized stream is included directly, along with instructions on how to recreate the data if needed in the future.

Comment by Chouser [ 18/Jul/17 11:54 AM ]

Alex, I just realized (by looking though this ticket's history) that I unintentionally overwrote your Description update last night. Not sure how that happened, but I apologize. Feel free to change it back or take bits from mine and add it to yours, or whatever else you feel is appropriate.





[CLJ-2203] Cannot create sets containing different empty collection, which prevents creating useful specs Created: 10/Jul/17  Updated: 10/Jul/17  Resolved: 10/Jul/17

Status: Closed
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: Declined Votes: 0
Labels: None
Environment:

[org.clojure/clojure "1.9.0-alpha17"]
[org.clojure/clojurescript "1.9.542"]



 Description   

In Clojure:

#{'() []}

Expected: No errors (the above works in Clojurescript)

Actual: "IllegalArgumentException Duplicate key: () clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:56)"

Motivation: I am trying to define a spec for the "into" argument to "s/coll-of" e.g.

(require '[clojure.alpha.spec :as s])
(s/def :coll-of/into #{'() [] {}})

It is possible to define this spec in Clojurescript, but not Clojure. Using a set to define a spec is idiomatic, but if this is a corner case, it would be fine to use a new "one-of" spec operation that is more verbose than a new set, works the same way when used in a spec, but avoids these cases.



 Comments   
Comment by Alex Miller [ 10/Jul/17 1:22 PM ]

In Clojure, sequential collections are compared in the same "equality partition" - that is lists, vectors, sequences, and potentially other custom sequential collections compare as equal based on their contents, not based on the collection type. This design decision has many subtle tradeoffs but enables many important aspects of using Clojure collections and sequences together in seamless ways. There are no plans to change this, so I am declining this ticket, as it is working as designed.

Regarding ClojureScript, I would start with the typical expectation that its behavior should match Clojure, but also take into account the specifics of the host such that there might be good reasons why this behavior is different (but I don't know enough to say either way).

For this particular spec, I would recommend a spec like (s/and coll? empty?). But please note that it's not possible in most cases to actually instrument spec itself with specs because in many cases this will create an infinite recursion of checks. I have written specs for the spec namespace but in general I did not find it workable in practice to actually use them so we have chosen not to provide them in general. Specs for spec forms are a work in progress tracked in CLJ-2112.





[CLJ-2202] coll-of :min-count and :gen-max used together cause collections that are too large to be generated Created: 10/Jul/17  Updated: 10/Jul/17

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

Type: Defect Priority: Minor
Reporter: Sebastian Oberhoff Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

1.9.0-alpha16


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

 Description   

This should specify a spec whose generator always returns collections of size 4 or 5 but instead it generates collections of size 4 to 8:

(require '[clojure.spec.alpha :as s] '[clojure.spec.gen.alpha :as gen])
(map count (gen/sample (s/gen (s/coll-of char? :min-count 4 :gen-max 5))))
;; (4 7 8 8 4 7 4 5 5 7)

Cause: The max logic in s/every-impl is: (c/or max-count (max gen-max (c/* 2 (c/or min-count 0)))). If there is a max-count it's used, otherwise the larger of gen-max or 2*min-count is used. In this case, 2*min-count is 8. Seems like we should never generate more than gen-max though, so propose changing this logic to: (c/or max-count gen-max (c/* 2 (c/or min-count 0))).

Patch: clj-2202.patch






[CLJ-2201] proxy-super is not threadsafe, it should be made safe or documented to be unsafe Created: 05/Jul/17  Updated: 05/Jul/17

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

Type: Enhancement Priority: Major
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Coming from java you might expect proxy-super to be pretty innocuous, but proxy-super operates by mutating the proxy object then restoring it after the call to proxy-super is invoked. This can lead to very weird behavior. If you have a proxy with method M, which invokes proxy-super, then while that proxy-super is running all calls to M on that proxy object will immediately invoke the super M not the proxied M.

Actually making proxy-super safe (not just threadsafe, but also safe when invoked later on in the same callstack) seems like it might be really hard, but it would be nice. Alternatively some blinking hazard lights in the docstring might be a good idea.






[CLJ-2200] for macro does not support multiple body expr Created: 05/Jul/17  Updated: 05/Jul/17  Resolved: 05/Jul/17

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

Type: Enhancement Priority: Trivial
Reporter: Isaac Zeng Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Clojure


Attachments: Text File 0001-let-macro-for-support-multiple-body-expr.patch    
Patch: Code

 Description   

it is not convenient for write multiple expression in for body,
from this now, we must explicit use `do` form, eg.

this form not supported

(for [i (range 10)]
  (prn i)
  (inc i))

we must use `do` wrap body

(for [i (range 10)]
  (do
    (prn i)
    (inc i)))


 Comments   
Comment by Daniel Stockton [ 05/Jul/17 5:10 AM ]

for is list comprehension. There is doseq for side effects: https://clojuredocs.org/clojure.core/doseq

Comment by Isaac Zeng [ 05/Jul/17 6:57 AM ]

@danielstockton

In fact, Many times, I found it will more convenient if for support this feature.

eg.
I want to see what happened in for loops(as normally, I use prn),

I'm also asked many folks, they thought it is better if for has this feature.

Comment by Nicola Mometto [ 05/Jul/17 8:34 AM ]

This is by design to discourage side-effects in its body, not only `for` is list comprehension and not a loop construct, but it's also lazy so if we were to facilitate side-effecty bodies in for, many would trip up on the laziness.

Comment by Alex Miller [ 05/Jul/17 8:41 AM ]

Declined, per the reasons already listed.





[CLJ-2199] Error attempting to unform unconformed keys (no :conform-keys opt) Created: 05/Jul/17  Updated: 05/Jul/17

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

Type: Defect Priority: Minor
Reporter: Daniel Stockton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Vetted

 Description   

Minimal failing case:

(s/def ::key-spec (s/or :kw keyword? :str string?))
(s/def ::map-spec (s/map-of ::key-spec identity))
(println (s/unform ::map-spec (s/conform ::map-spec {:a :b})))
java.lang.UnsupportedOperationException: nth not supported on this type: Keyword

If keys are not conformed, we should also not attempt to unform them.






[CLJ-2198] int-in-range? doesn't check for int-ness Created: 01/Jul/17  Updated: 01/Jul/17  Resolved: 01/Jul/17

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

Type: Defect Priority: Major
Reporter: Frederic Merizen Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: spec
Environment:

Clojure 1.9.0-alpha17
Spec.alpha 0.1.123



 Description   
(require '[clojure.spec.alpha :as s])
(s/int-in-range? 1 4 1.5)
=> true

I expected the result to be false, because 1.5 is not an integer.

Cause of the bug: in the following snippet, int? is always truthy. It should read (int? val) instead.

(defn int-in-range?
  "Return true if start <= val, val < end and val is a fixed
  precision integer."
  [start end val]
  (c/and int? (<= start val) (< val end)))

The bug is fairly harmless in practice because int-in, the main client of int-in-range?, 'redundantly' checks for int-ness:

(defmacro int-in
  "Returns a spec that validates fixed precision integers in the
  range from start (inclusive) to end (exclusive)."
  [start end]
  `(spec (and int? #(int-in-range? ~start ~end %))
     :gen #(gen/large-integer* {:min ~start :max (dec ~end)})))


 Comments   
Comment by Alex Miller [ 01/Jul/17 2:37 PM ]

Dupe of https://dev.clojure.org/jira/browse/CLJ-2167 - fix should go in soon.





[CLJ-2197] instrument :stub doesn't use :gen override 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: Defect Priority: Major
Reporter: Michael Glaesemann Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec


 Description   

`instrument` doesn't respect `:gen` override for `:stub`.

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

;; [org.clojure/spec.alpha "0.1.123"]

;; The goal is to stub functions which require some kind external
;; dependency, such as a service or other I/O.

(defprotocol Y
  (-do-y [r]))

(def y? (partial satisfies? Y))
(s/def ::y y?)

;; Protocol methods can't be spec'd, so wrap them in a function.

(defn do-y [r]
  (-do-y r))

(s/fdef do-y :args (s/cat :y-er ::y))

;; Example of the protocol implementation that we're going to stub.

(defrecord BadYer []
  Y
  (-do-y [_] (throw (Exception. "can't make me!"))))

;; Confirm BadYer instances are valid with respect to the protol spec.

(s/valid? ::y (->BadYer))
;; => true

;; And confirm BadYer instances will throw when called.

(try
  (do-y (->BadYer))
  (catch Exception e
    (.getMessage e)))
;; => "can't make me!"


(def y-gen (gen/return (->BadYer)))

;; Confirm generator works as expected:

(gen/sample y-gen 1)
;; => (#spec_ex.core.BadYer{})

;; We want to stub `do-y`, providing y-gen as a generator for `::y`

(try
  (stest/instrument `do-y {:stub #{`do-y}
                           :gen {::y (fn [] y-gen)}})
  (catch Exception e
    (ex-data e)))
;; => #:clojure.spec.alpha{:path [:y-er], :form :spec-ex.core/y, :failure :no-gen}

;; However, we *can* stub `do-y` if we replace its spec.

(stest/instrument `do-y
                  {:stub #{`do-y}
                   :spec {`do-y (s/fspec
                                  :args (s/cat :y-er (s/with-gen ::y
                                                       (fn [] y-gen))))}})
;; => [spec-ex.core/do-y]





[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-2195] integer (32-bit) bit operations not available Created: 30/Jun/17  Updated: 30/Jun/17  Resolved: 30/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: Phillip Lord Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: interop


 Description   

It would be nice if Clojure provided implementations for all Java
operators, using primitive types. While Clojures numbers handling is
nice, there are algorithms for which it is a pain.

Clojure currently provides a number of functions to deal with this:
unchecked-add and unchecked-add-int, for instance. But, not all of the
Java operators are accessible in this way, at least to my knowledge;
specifically, the "&" and "|" operators have no function
equivalents. Actually, they are still present in source (Numbers.java)
but have been commented out. I can't really understand the reason for
this omission.



 Comments   
Comment by Andy Fingerhut [ 30/Jun/17 9:17 AM ]

Are these functions in clojure.core not what you are hoping to have?

user=> (pprint (apropos "bit-"))
(clojure.core/bit-and
clojure.core/bit-and-not
clojure.core/bit-clear
clojure.core/bit-flip
clojure.core/bit-not
clojure.core/bit-or
clojure.core/bit-set
clojure.core/bit-shift-left
clojure.core/bit-shift-right
clojure.core/bit-test
clojure.core/bit-xor
clojure.core/unsigned-bit-shift-right)

Comment by Alex Miller [ 30/Jun/17 9:32 AM ]

Although it supports all primitives for the purposes of Java interoperability, pure Clojure uses only long and double primitives. Clojure will promote ints to longs and floats to doubles as needed.

There are any plans to ever offer bit ops for 32-bit integers. You can use longs and the existing functions to get primitive-level long bit ops using `bit-and`, `bit-or`, `bit-xor`, `bit-not`, `bit-shift-left`, `bit-shift-right`, `unsigned-bit-shift-right`, etc.

Comment by Phillip Lord [ 30/Jun/17 10:25 AM ]

@Andy Fingerhut

user> (type (bit-and (int 0)(int 0)))
java.lang.Long

@Alex Miller Presume you mean "There are not any plans"...

I don't understand the logic though; there are already explicit functions both in clojure.core supporting int operations for add in both Numbers.java and core.clj, functions in Number.java for shiftLeftInt, shiftRightInt, and unsignedShiftRight. But &, |, ^ and ~ cannot be replicated, at least as far as I can tell. It seems a bit half-and-half. Why do some of operators in clojure, some only in Java, and some not at all?

We can work around this, of course, but if we have to port these operations into Java, it's probably easier to port the entire algorithm.

Comment by Alex Miller [ 30/Jun/17 11:18 AM ]

Sorry, yes - "there are not any plans".

It is difficult for me to go back in time (before I worked on Clojure) and summarize the results of months of design effort on Rich's part which presumably considered many tradeoffs and criteria. The end result though is: pure Clojure uses only long and double primitives and that's where we provide operations. There are a few cases where int versions of some functionality is available - those are likely either vestigial, or are used inside the Clojure implementation somewhere but if they're not surfaced in core, they're not part of the API.

Comment by Phillip Lord [ 30/Jun/17 11:36 AM ]

They are in the API. unchecked-add-int, unchecked-multiply-int are all surfaced in core. Just the arithmetic operators, but not the bitwise.

Anyway, thanks for the explanation; at least, now the situation is clear. I can either port the entire algorithms or just these operators out to Java.

Comment by Alex Miller [ 30/Jun/17 11:53 AM ]

The Java classes are the implementation, not the API.

Comment by Phillip Lord [ 30/Jun/17 12:42 PM ]

user=> clojure.core/unchecked-add-int
#object[clojure.core$unchecked_add_int 0x479395d7 "clojure.core$unchecked_add_int@479395d7"]
user=> (clojure.core/unchecked-add-int 1 2)
3
user=> (type (clojure.core/unchecked-add-int 1 2))
java.lang.Integer

So, int operations in the API, yes? But just for the arithmetic operators and not for the bitwise ones.

Comment by Ghadi Shayban [ 30/Jun/17 1:59 PM ]

Symmetry is not one of the stronger API design motivations

Comment by Alex Miller [ 30/Jun/17 2:59 PM ]

Sorry, yes - some int operations are in the Clojure api. There are no plans to add the int bit ops.





[CLJ-2194] Spec metadata support Created: 30/Jun/17  Updated: 07/Jul/17

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

Type: Enhancement Priority: Major
Reporter: Max Penet Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: spec


 Description   
  1. Spec metadata support

Problem: Currently there is no way to attach metadata to a spec

It would be nice to be able to add a docstring (the primary use case),
or potentially useful information about usage of that spec in different
contexts (static code analyser, custom conversion/coercion, how it
relates to a particular db schema, human readable error message
template, domain specific concerns or even clj.spec itself later, etc...).

In theory one can today create his own meta-data registry and handle
this at the library level, and that's what a few spec related project
already do, but it would be nicer to have a unified/standard way to do
this. By default it would make sense to add support docstrings for a
start. It could take the form of an extra argument of:

;; at least the following two
(s/def ::foo "Something that's a foo" any?)
(s/def ::foo string? {:doc "Something that's a foo" :cassandra-type :varchar})

;; potentially these depending on the implementation
(s/spec #() :gen ... :meta ...)
(with-meta (s/spec ...))

There are a few ways to implement this, with various pros/cons:

  • Implement the IMeta protocol:
    This seems like the clean approach, meta data would/could be supported
    at any Spec level (ex a non registered spec predicate, a Set spec and
    so on). The implementation would require a fair amount changes to the
    current code tho. Mostly adding a meta parameter to the various
    *-spec-impl macros and sugar at `s/def` and derivatives' level.
    A tricky part with that approach is that registered specs that reference
    another spec are just a "link" (a keyword), so we have nowhere to add
    metadata right now.
    They could be reified, return a "pointer" to their original spec and hold
    metadata at their own level.
  • a simple registry (similar to the spec registry, or shared in the main spec registry):
    Basically a map of spec-kw -> metadata if in a separate registry, or integrated into the
    main registry somehow.
    That's the easy approach, only registered spec would be supported, metadata is separated
    from the rest, would keep the Spec instances a bit lighter. Spec referencing other specs
    could have their own metadata.
    As mentioned this could be done in a separate registry or added to a spec value in the main spec
    registry.

It seems to be the IMeta is probably the better solution, we'd leverage the existing "meta" api.



 Comments   
Comment by Nicola Mometto [ 30/Jun/17 4:20 AM ]

(s/def ^{:doc "Something that's a foo" :cassandra-type :varchar} ::foo string?)
is not valid clojure, you can't add metadata to a keyword

Comment by Max Penet [ 30/Jun/17 4:26 AM ]

Changed example as per Nicolas' comment.

Comment by Andy Fingerhut [ 04/Jul/17 10:44 AM ]

Related to (if not a dup of) CLJ-1965

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

This is related to but not the same as CLJ-1965 - the scope here is larger to potentially support any meta.





[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-2192] When data fails to conform to `map-of` spec, `:in` path does not point to the invalid (inner) value Created: 27/Jun/17  Updated: 12/Jul/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/clojure "1.9.0-alpha17", org.clojure/clojurescript "1.9.542", org.clojure/core.specs.alpha "0.1.10"



 Description   

Repro:

(require '[clojure.spec.alpha :as s]
(s/def :foo/user-map (s/map-of string? int?))
(s/explain-data :foo/user-map {"hi" "foo"})
;; Actual value:
;; #:cljs.spec.alpha{:problems
;;                 ({:path [1],
;;                   :pred int?,
;;                   :val "foo", 
;;                   :via [:foo/user-map], 
;;                   :in ["hi" 1]})}
;; Expected: the `:in` value to be ["hi"] ? 
(s/explain-data :foo/user-map {:hi 2})
;; Actual value: 
;; #:cljs.spec.alpha{:problems
;;                 ({:path [0],
;;                   :pred string?,
;;                   :val :hi, 
;;                   :via [:foo/user-map], 
;;                   :in [:hi 0]})}
;; Expected: I'm not sure, since a path can't "point to" a key

Motivation: given some top-level data (in this case, `{"hi" "foo"}`) and an `:in` path, I would like to be able to find the problematic data (in this case, `"foo"`).

In the case where the value of a map does not conform, the `:in` path is not compatible with functions like `get-in`, but it could be.

In the case where the key of a map does not conform, there is no way to "point to" a key using `get-in`, so I'm not sure what the right fix is.

I don't know that compatibility with `get-in` is a requirement: if spec provided a function that accomplished the same thing with "spec" paths (i.e. ones that could point to keys), that would be fine.



 Comments   
Comment by juan pedro monetta sanchez [ 12/Jul/17 9:58 AM ]

I think that more important than get-in compatible is a way of matching :clojure.spec.alpha/problems inside :clojure.spec.alpha/value independent of the spec that lead to the problem.
For this I think it's important to know if the problem is with the key or the value.

Currently s/map-of reports a path taking into account the map-entry vec, so 0 will be the key and 1 the value.

The problem with what I'm trying to implement is the :in is s/keys which only reports the key.

So when you see a problem in [::k 1] you don't know if it's a problem in the map value or the value is a seq and the problem is in the value at pos 1 in that seq.





[CLJ-2191] Docstring clarifications for clojure.spec.test.alpha/check Created: 27/Jun/17  Updated: 27/Jun/17  Resolved: 27/Jun/17

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

Type: Enhancement Priority: Trivial
Reporter: Michael Nygard Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: docstring


 Description   

The docstring for `check` refers to `::stc/opts` but that alias is only visible in the source code for clojure.spec.test.alpha. I recommend expanding the alias in the docstring so it says `:clojure.spec.test.check/opts`.

Likewise for the reference to `::stc/ret` in the same docstring.



 Comments   
Comment by Alex Miller [ 27/Jun/17 11:10 AM ]

The check docstring says "The opts map includes the following optional keys, where stc
aliases clojure.spec.test.check:"

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

Closing as unnecessary per separate conversation.

Comment by Michael Nygard [ 27/Jun/17 11:17 AM ]

Yes, it definitely does say that. I didn't read closely enough. Feel free to close.





[CLJ-2190] clojure.spec.alpha/exercise-fn should emit a bettor error message when no implementation is found for a symbol Created: 27/Jun/17  Updated: 27/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: Abhirag Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, spec
Environment:

Clojure 1.9.0-alpha17
test.check 0.10.0-alpha2


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

 Description   

Here we get a NullPointerException because although we do have a spec for my-reverse, we don't have an implementation for it, a more descriptive error message would help.

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

(s/fdef foo :args (s/cat :x int?) :ret int?)
=> user/foo

(s/exercise-fn `foo)
NullPointerException   clojure.core/apply (core.clj:657)

Proposed: Check for a nil function and throw.

Patch: clj-2190.patch






[CLJ-2189] gen-class does not preserve parameter names of overridden methods Created: 25/Jun/17  Updated: 25/Jun/17  Resolved: 25/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: Maxim Neverov Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: interop

Attachments: PNG File ScreenShot.png    
Approval: Triaged

 Description   

Parameters names are not preserved during classes generation. It relates to interfaces and abstract classes clojure class inherited from as well as the methods declared in :methods part of the gen-class.

It would be useful to preserve names so that java programmers that use clojure libraries wouldn't be confused.

Steps to reproduce:
1. Declare java interface or abstract class with methods to implement in clojure.
2. Use gen-class to generate jar:

(ns foo.bar.core
  (:gen-class
    :name foo.bar.ClientImpl
    :extends foo.bar.AbstractClient
    :main false
    :methods [[method [String] void]]))

3. Add resulted jar in other java project.
4. Methods parameters looks like s1, aLong1 etc (like it shown in the attached screen shot).

Complete example is here



 Comments   
Comment by Maxim Neverov [ 25/Jun/17 1:46 PM ]

:uberjar did the trick. Ticket can be closed. Sorry for inconvenience.





[CLJ-2188] clojure.core/slurp does not mark its return type as String Created: 23/Jun/17  Updated: 24/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJ-2188-v1.patch    
Patch: Code
Approval: Prescreened

 Description   

Given that slurp always returns a string (or throws), a user might expect that calling a string method on it will not reflect. However, its return type is not hinted, so calling, say, (.getBytes (slurp "foo")) will reflect unless the user hints. If we hint in clojure.core, the user won't have to.

user=> (set! *warn-on-reflection* true)
true
user=> (.length (slurp "pom.xml"))
Reflection warning, NO_SOURCE_PATH:3:1 - reference to field length can't be resolved.
9218

Patch: CLJ-2188-v1.patch

Prescreened by: Alex Miller






[CLJ-2187] if-seq and if-pred for more flexible conditional assignment Created: 23/Jun/17  Updated: 23/Jun/17  Resolved: 23/Jun/17

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

Type: Feature Priority: Major
Reporter: Michael Zavarella Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

The current `if` macros are very useful but are limited in their use case. I wrote a couple of macros that I've found useful since writing them and I think other would too.

if-seq has nice to have for when you have a collection and want 'else' to evaluate if the collection is false-y or empty.

if-pred has been nice as a 'go-to' and is one of my most used functions/macros.

(defmacro if-seq
  "bindings => binding-form test

  If test is a seq, evaluates then with binding-form bound to the value of
  test, if not, yields else"
  ([bindings then]
   `(if-seq ~bindings ~then nil))
  ([bindings then else & oldform]
   (assert-args
    (vector? bindings) "a vector for its binding"
    (nil? oldform) "1 or 2 forms after binding vector"
    (= 2 (count bindings)) "exactly 2 forms in binding vector")
   (let [form (bindings 0) tst (bindings 1)]
     `(let [temp# ~tst]
        (if (seq temp#)
          (let [~form temp#]
            ~then)
          ~else)))))

(defmacro if-pred
  ([pred bindings then]
   `(if-pred ~pred ~bindings ~then nil))
  ([pred bindings then else & oldform]
   (assert-args
    (vector? bindings) "a vector for its binding"
    (nil? oldform) "1 or 2 forms after binding vector"
    (= 2 (count bindings)) "exactly 2 forms in binding vector")
   (let [form (bindings 0) tst (bindings 1)]
     `(let [temp# ~tst]
        (if (~pred temp#)
          (let [~form temp#]
            ~then)
          ~else)))))


 Comments   
Comment by Alex Miller [ 23/Jun/17 9:00 AM ]

Hi Michael, thanks for the suggestion. I don't think we'll add these to core, but seems like a good addition to a utility library.





[CLJ-2186] ::s/map-bindings definition is underspecified Created: 22/Jun/17  Updated: 23/Jun/17

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

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

Attachments: Text File clj-2186.patch    
Patch: Code
Approval: Screened

 Description   

:clojure.core.specs.alpha/map-bindings` is less strict than expected. It specifies `:into {}`, but not `:kind map?` which means non-maps conform.

(s/conform ::clojure.core.specs.alpha/map-binding-form '[[foo bar]])
=>
[[foo bar]]

This is also an issue because of the next line:

(s/def ::map-binding-form (s/merge ::map-bindings ::map-special-binding))

s/merge takes two maps, and ::map-bindings is not required to be a map.

Patch: clj-2186.patch

Screened by: Alex Miller (apply to core.specs.alpha)



 Comments   
Comment by Sameer Rahmani [ 23/Jun/17 11:01 AM ]

On clojure 1.9.0-alpha17 :

user=> (s/conform ::clojure.core.specs.alpha/map-binding-form '[[foo bar]])
:clojure.spec.alpha/invalid




[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-2184] propagate metadata in doto forms Created: 20/Jun/17  Updated: 20/Jun/17

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

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

Attachments: Text File CLJ-2184-patch-00.patch    
Patch: Code
Approval: Prescreened

 Description   

The doto macro currently produces lists without metadata, which among other things means errors and warnings are misleading or less specific than they could be. For example, this code generates reflection warnings, but the warnings point to the beginning of the doto block itself:

(set! *warn-on-reflection* true)

(def a "a")
(def b (int \b))

(doto "abc"
  (.indexOf a)
  (.indexOf b))

Note the line and column numbers in these warnings:

Reflection warning, t1.clj:6:1 - call to method indexOf on java.lang.String can't be resolved (argument types: unknown).
Reflection warning, t1.clj:6:1 - call to method indexOf on java.lang.String can't be resolved (argument types: unknown).

A more specific and accurate output would look like this:

Reflection warning, t1.clj:7:3 - call to method indexOf on java.lang.String can't be resolved (argument types: unknown).
Reflection warning, t1.clj:8:3 - call to method indexOf on java.lang.String can't be resolved (argument types: unknown).

Similar macros like -> take pains to propagate metadata from the users forms to the macro's output. This is more important for those macros because of how they interact with type hints.
Since the return values of the interior doto forms are ignored, the metadata is somewhat less important, but the example above shows the the propagation would still be valuable.

Patch: CLJ-2184-patch-00.patch

Prescreened by: Alex Miller



 Comments   
Comment by Alex Miller [ 20/Jun/17 11:20 AM ]

Patches welcome....

Comment by Chouser [ 20/Jun/17 11:24 AM ]

Attached "CLJ-2184-patch-00.patch" which uses a single with-meta call in doto to copy the metadata from each user-supplied form to the output form.





[CLJ-2183] `cat` specs should verify value is sequential Created: 08/Jun/17  Updated: 30/Jun/17

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

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

[org.clojure/clojure "1.9.0-alpha17"]
[org.clojure/spec.alpha "0.1.109"]


Attachments: Text File clj-2183.patch    
Patch: Code and Test
Approval: Screened

 Description   

Regex op specs can currently pass with maps or sets (which are unordered) but may give confusing errors.

(require '[clojure.spec.alpha :as s])
(s/def ::kv (s/cat :k keyword? :v any?))
(s/explain ::kv {"foo" "bar"})
In: [0] val: ["foo" "bar"] fails spec: :user/kv at: [:k] predicate: keyword?

Cause: Conforming fails because the first value of the map (the tuple pair `["foo" "bar"]`) is not a keyword

Proposed: Regex op specs currently check `coll?`, which will pass unordered collections like sets or maps - this is unlikely to be useful for positional regex specs. Propose to narrow that check to `sequential?`. On failure, use an explain pred that describes the actual check (the current one just repeats the regex spec instead).

With the patch, the message actually tells you the actual predicate that is failing (the sequential? check):

val: {"foo" "bar"} fails spec: :user/kv predicate: (or (nil? %) (sequential? %))

Patch: clj-2183.patch

Screened by: Chouser - while making unordered colls invalid is the intent of the patch, a gray area is that of sorted colls (sorted sets, etc). These could have been matched with the prior impl, but will not be after the change. See comments for more examples.



 Comments   
Comment by Chouser [ 20/Jun/17 11:44 PM ]

Note, I believe this is a breaking change. Before the patch, this works:

(s/def ::dt (s/cat :datep (s/spec (s/cat :datek #{::date}, :datev number?))
                   :timep (s/spec (s/cat :timek #{::time}, :timev number?))))

(s/explain ::dt {::date 10, ::time 20})
;; Success!

After the patch, it fails:

(s/explain ::dt {::date 10, ::time 20})
;; val: #:user{:date 10, :time 20} fails spec: :user/dt predicate: (or (nil? %) (sequential? %))
;; :clojure.spec.alpha/spec  :user/dt
;; :clojure.spec.alpha/value  #:user{:date 10, :time 20}

However, even without the patch such specs will match only as long as the map (or set) entries are in the expected order. For hash-maps and hash-sets (or collections like array-maps that may be promoted to hash-maps), this is unpredictable and the patch arguably only "breaks" things that were at risk of breaking anyway.

Sorted collections are a little dicier. Without the patch, this arguably reasonable spec works reliably for sorted sets:

(s/def ::ab (s/cat :a (s/? #{"a"}) :b (s/? #{"b"})))
(s/explain ::ab (sorted-set "a" "b"))
;; Success!

With the patch, the sorted-set doesn't match because it's not a sequential collection, thus a breaking change.

Fortunately spec is still alpha so breaking changes are ok, and specs that intend to match sorted collections have several other options for doing so, especially the spec macros meant for non-sequential collections, like so:

(s/def ::ab (s/coll-of #{"a" "b"}))
Comment by Alex Miller [ 21/Jun/17 1:39 AM ]

Yes, the idea here is that this constrains the scope of what regex op specs support, so the "breaking" part of it is intentional.

Sorted colls is an interesting question, seems like a gray area. Personally, I'd be happy to rule it out, but maybe Rich would disagree. For something like this, it's best to note it as screener notes in the description as Rich doesn't necessarily read all the comments. I'll add it as an example.





[CLJ-2182] s/& does not check preds if regex matches empty collection Created: 08/Jun/17  Updated: 29/Jun/17

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

Type: Defect Priority: Critical
Reporter: Shogo Ohta Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File clj-2182-2.patch     Text File clj-2182.patch    
Patch: Code and Test
Approval: Screened

 Description   

In the following example, the ::x key is required in the map but it will conform without error for an empty collection:

(s/def ::x integer?)
(s/conform (s/keys* :req-un [::x]) [])
;; nil, expected error
(s/explain (s/keys* :req-un [::x]) [])
;; Success!, expected explanation

Cause: At the moment, (s/keys* :req-un [::x]) is expanded to a form equivalent to the following one:

(s/& (s/* (s/cat ::s/k keyword? ::s/v))
     ::s/kvs->map
     (s/keys :req-un [::x]))

The issue seems to be in the implementation of s/&, specifically the ::amp branch of accept-nil? which expects that either the conformed regex returns empty or that the preds are not invalid. This seems like a false assumption that an empty conformed regex ret does not require the s/& preds to be valid. In this case we are using a conformer to transform the input and do another check, so it's certainly not valid here.

Proposed: Modify s/& to always validate the preds when accepting nil.

user=> (s/conform (s/keys* :req-un [::x]) [])
:clojure.spec.alpha/invalid

user=> (-> (s/explain-data (s/keys* :req-un [::x]) []) ::s/problems first :pred)
(clojure.core/fn [%] (clojure.core/contains? % :x))

Patch: clj-2182-2.patch



 Comments   
Comment by Alex Miller [ 29/Jun/17 4:20 PM ]

-2 patch includes a test





[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: 0
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-2179] s/inst-in and s/int-in generators should have uniform distribution not biased towards min value Created: 06/Jun/17  Updated: 06/Jun/17

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: gen-class, spec

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

 Description   

The s/inst-in and s/int-in generators are based on gen/large-integer* which grows from 0.

(require '[clojure.spec.alpha :as s] '[clojure.spec.gen.alpha :as gen])
(gen/sample (s/gen (s/int-in 0 100)))
;;=> (1 0 1 1 1 0 1 1 72 1)

(gen/sample (s/gen (s/inst-in #inst "2001-01-01" #inst "2001-12-31")))
;;=> (#inst "2001-01-01T00:00:00.000-00:00" #inst "2001-01-01T00:00:00.000-00:00" #inst "2001-01-01T00:00:00.001-00:00" #inst "2001-01-01T00:00:00.001-00:00" ...)

Proposed: Instead, s/inst-in should use a uniform distribution generator:

After on same:

(26 16 65 96 63 37 31 4 94 9)

(#inst "2001-03-03T04:51:43.702-00:00" 
 #inst "2001-07-25T07:13:03.224-00:00" 
 #inst "2001-03-31T18:28:41.625-00:00" 
 #inst "2001-04-17T19:33:14.176-00:00" 
 #inst "2001-01-14T07:03:08.521-00:00" 
 #inst "2001-06-06T09:52:03.421-00:00" ...)

Patch: clj-2179.patch






[CLJ-2178] s/& explain-data :pred problem Created: 05/Jun/17  Updated: 29/Jun/17

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

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

Attachments: Text File clj-2178.patch    
Patch: Code and Test
Approval: Screened

 Description   

As reported in CLJ-2175, s/& has some issues with reporting valid resolved preds in explain-data:

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

(-> (s/explain-data (s/& int? even?) []) ::s/problems first :pred)
;;=> #function[clojure.core/int?]
;;EXPECTED: clojure.core/int?

(-> (s/explain-data (s/& int? even?) [0 2]) ::s/problems first :pred)
;;=> (clojure.spec.alpha/& #function[clojure.core/int?] clojure.core/even?)
;;EXPECTED: (clojure.spec.alpha/& clojure.core/int? clojure.core/even?)

Problem: s/& was not capturing the re form. Added a new :amp key to carry the re form (similar to :maybe key in s/?) and used that for describe when necessary.

Patch: clj-2178.patch






[CLJ-2177] s/keys explain-data :pred problem Created: 05/Jun/17  Updated: 29/Jun/17

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

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

Attachments: Text File clj-2177.patch    
Patch: Code and Test
Approval: Screened

 Description   

As reported in CLJ-2175, s/keys has an issue with reporting a valid resolved pred in explain-data:

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

(-> (s/explain-data (s/keys :req [::x]) :a) ::s/problems first :pred)
;;=> map?   
;;EXPECTED: clojure.core/map?

Patch: clj-2177.patch



 Comments   
Comment by Shogo Ohta [ 05/Jun/17 6:31 PM ]

Missing the patch?





[CLJ-2176] s/tuple explain-data :pred problems Created: 05/Jun/17  Updated: 29/Jun/17

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

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

Attachments: Text File clj-2176.patch    
Patch: Code and Test
Approval: Screened

 Description   

As reported in CLJ-2175, s/tuple has some issues with reporting valid resolved preds in explain-data:

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

(-> (s/explain-data (s/tuple int?) :a) ::s/problems first :pred)
;;=> vector?   
;;EXPECTED: clojure.core/vector?

(-> (s/explain-data (s/tuple int?) []) ::s/problems first :pred)
;;=> (clojure.core/= (clojure.core/count %) 1)  
;;EXPECTED: (clojure.core/fn [%] (clojure.core/= (clojure.core/count %) 1))

Patch: clj-2176.patch






[CLJ-2175] Some collection specs have inconsistent forms of :pred in their explain data Created: 02/Jun/17  Updated: 05/Jun/17  Resolved: 05/Jun/17

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

Type: Defect Priority: Major
Reporter: Shogo Ohta Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: spec

Approval: Vetted

 Description   

Here are the results of my investigation (you can reproduce them by (s/explain-data <spec> <input>)):

# <spec> <input> <expected> :pred
1. (s/tuple integer?) :a clojure.core/vector? vector?
2. (s/tuple integer?) [] (clojure.core/fn [%] (clojure.core/= (clojure.core/count %) 1)) (clojure.core/= (clojure.core/count %) 1)
3. (s/keys :req [::x]) :a clojure.core/map? map?
4. (s/keys :req [::x]) {} (clojure.core/fn [%] (clojure.core/contains? % :user/x)) (ditto)
5. (s/coll-of integer?) :a clojure.core/coll? (ditto)
6. (s/? integer?) :a (clojure.spec.alpha/? clojure.core/integer?) (ditto)
7. (s/? integer?) [1 2] (clojure.spec.alpha/? clojure.core/integer?) (ditto)
8. (s/* integer?) :a (clojure.spec.alpha/* clojure.core/integer?) (ditto)
9. (s/+ integer?) :a (clojure.spec.alpha/+ clojure.core/integer?) (ditto)
10. (s/+ integer?) [] clojure.core/integer? (ditto)
11. (s/& integer? even?) [] clojure.core/integer? #function[clojure.core/integer?]
12. (s/& integer? even?) [0 2] (clojure.spec.alpha/& clojure.core/integer? clojure.core/even?) (clojure.spec.alpha/& #function[clojure.core/integer?] clojure.core/even?)
13. (s/cat :i integer?) [] clojure.core/integer? (ditto)
14. (s/cat :i integer?) [1 2] (clojure.spec.alpha/cat :i clojure.core/integer?) (ditto)
15. (s/alt :i integer? :s string?) [] (clojure.spec.alpha/alt :i clojure.core/integer? :s clojure.core/string?) (ditto)
16. (s/alt :i integer? :s string?) [1 2] (clojure.spec.alpha/alt :i clojure.core/integer? :s clojure.core/string?) (ditto)

I think there are 4 different types of problems to be resolved here (marked with ):

  • Problem 1 (1. & 3.): symbols representing fn name should be resolved (as in 5.)
  • Problem 2 (2.): compound expressions should be wrapped to fn forms (as in 4.)
  • Problem 3 (11. & 12.): functions shouldn't be put into :pred as they are


 Comments   
Comment by Alex Miller [ 02/Jun/17 8:22 AM ]

Thanks, this is great. It would be helpful to have an "expected" column as well that lists what the :pred should be.

On 10 and 13 (Problem 3) - I don't agree that these are a problem. If you look at the problem, it's a complaint about insufficient input and the pred is being used to indicate the kind of input that was expected.

Comment by Shogo Ohta [ 02/Jun/17 7:42 PM ]

It would be helpful to have an "expected" column as well that lists what the :pred should be.

Sure. I will.

On 10 and 13 (Problem 3) - I don't agree that these are a problem. If you look at the problem, it's a complaint about insufficient input and the pred is being used to indicate the kind of input that was expected.

Yes, I think that's the most controversial point. Both the options look reasonable to me. But I'm not sure the results of the case 10. and 13. are really consistent with that of 15. from your stance.

Comment by Alex Miller [ 05/Jun/17 7:43 AM ]

10 and 13 are not controversial - these are the expected results. I think it is consistent with 15 - in both cases something is expected and we are stating the most specific spec/pred that is expected.

Comment by Alex Miller [ 05/Jun/17 8:34 AM ]

I've logged:

Closing this now as a duplicate of those 3 isolated issues.

Comment by Shogo Ohta [ 05/Jun/17 9:13 AM ]

OK, thank you for your time!





[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: 01/Jun/17

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

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-2171] Default explain printer prints root val and spec Created: 31/May/17  Updated: 30/Jun/17

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

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

Attachments: Text File clj-2171.patch    
Patch: Code
Approval: Screened

 Description   

In CLJ-2085 (added in spec.alpha 0.1.123 / Clojure 1.9.0-alpha17) we added new attributes to the explain-data map. However, explain-printer reports all non-problem attributes (previously was used for reporting the arguments in an instrument failure) so we are now getting a lot more printing than desired.

Example (last 2 lines here are new):

user=> (s/explain #{1 2} 5)
val: 5 fails predicate: :clojure.spec.alpha/unknown
:clojure.spec.alpha/spec  #{1 2}
:clojure.spec.alpha/value  5

Proposed: I think we should only print the ::problems and the instrument explain should have some more work done on it anyways to improve it separately.

Patch: clj-2171.patch






[CLJ-2170] Several top-level forms have improperly-located docstrings Created: 29/May/17  Updated: 30/May/17

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

Type: Defect Priority: Minor
Reporter: Cameron Desautels Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File fix-improper-docstrings.patch    
Patch: Code
Approval: Prescreened

 Description   

A number of top-level forms have docstrings which are improperly-located within the defining form (viz. defn / defn- / defmacro), and thus are discarded rather than attached as proper metadata. I believe I have fixed all (10) instances within the project with my patch.

The following code demonstrates the problem and the efficacy of the patch:

(def doc-syms
  "Symbols missing documentation because it's incorrectly located
  within the defn / defn- / defmacro form."
  [#'clojure.core/group-by-sig
   #'clojure.pprint/setf
   #'clojure.pprint/unzip-map
   #'clojure.pprint/tuple-map
   #'clojure.pprint/rtrim
   #'clojure.pprint/ltrim
   #'clojure.pprint/prefix-count
   #'clojure.pprint/prerr
   #'clojure.pprint/prlabel
   #'clojure.set/bubble-max-key])

;; before patch
(every? nil?    (map (comp :doc meta) doc-syms)) ; => true

;; after patch
(every? string? (map (comp :doc meta) doc-syms)) ; => true

Prescreened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 29/May/17 10:26 PM ]

If your 'after patch' expression truly does evaluate to false, it doesn't give much confidence that your change fixed them all, does it? Shouldn't it evaluate to true? Perhaps it isn't true because of some behavior related to defn- and doc strings. I have not checked.

CLJ-1314 has a change like this for clojure.set/bubble-max-key only. If this change is committed, that issue could be closed, too.

Comment by Cameron Desautels [ 29/May/17 10:55 PM ]

Ha. Apologies. This is a typo. It evaluates to true, as I encourage anyone to test.

I'll amend it.

Comment by Cameron Desautels [ 29/May/17 11:01 PM ]

Hmm, perhaps I don't have permission to edit. The typo stands, for now.

To clarify for anyone reading: the issue description has a typo, not the patch.

Comment by Alex Miller [ 30/May/17 8:59 AM ]

Cameron Desautels I gave you edit rights.

Comment by Cameron Desautels [ 30/May/17 10:47 AM ]

Thank you. I have corrected the issue description.





[CLJ-2169] conj has out-of-date :arglists Created: 27/May/17  Updated: 27/May/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: None

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-2168] clojure.spec: :pred in explain for coll-of should have resolved symbols Created: 26/May/17  Updated: 01/Jun/17

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

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

0.1.123


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

 Description   

:pred should be resolved in explain problems like:

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

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

should be

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

Other examples:

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

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

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

Patch: clj-2168.patch



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

The same problem happens with s/every.

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

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

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

For example,

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

I'll file that as another ticket later.





[CLJ-2167] int-in-range? throws exception when val not an int Created: 26/May/17  Updated: 30/Jun/17

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

Type: Defect Priority: Major
Reporter: David Bürgin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

Clojure 1.9.0-alpha16
spec.alpha 0.1.123


Attachments: Text File int-in-range.patch    
Patch: Code
Approval: Screened

 Description   

The spec predicate int-in-range? throws an exception when not passed an int? value.

(require '[clojure.spec.alpha :as s])
(s/int-in-range? 0 10 :x)
ClassCastException clojure.lang.Keyword cannot be cast to java.lang.Number

Expected result: false

Cause: int-in-range? uses int? instead of (int? val), so that condition always evaluates true regardless of input.

Solution: Use (int? val) instead.

Patch: int-in-range.patch

Screened by: Alex Miller



 Comments   
Comment by Ghadi Shayban [ 26/May/17 1:05 PM ]

It's totally a typo & you should sign the CA. Sooner the better because there seems to be some spec bug fixing activity today





[CLJ-2166] instrument exception doesn't contain function name in ex-data Created: 24/May/17  Updated: 24/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: James Reeves Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: spec

Attachments: Text File clj2166.patch    
Patch: Code
Approval: Screened

 Description   

When an instrumented function fails, it throws an IExceptionInfo. The ex-data of this exception contains the arguments that failed, but not the function that was called.

(require '[clojure.spec.alpha :as s] '[clojure.spec.test.alpha :as stest])
(defn foo [x] (+ x 1))
(s/fdef foo :args (s/cat :x number?))
(stest/instrument `foo)
(foo "x")  ;; ExceptionInfo Call to #'user/foo did not conform to spec:
(ex-data *e)

{:clojure.spec.alpha/problems
 [{:path [:args :x], :pred number?, :val "x", :via [], :in [0]}],
 :clojure.spec.alpha/args ("x"),
 :clojure.spec.alpha/failure :instrument,
 :clojure.spec.test.alpha/caller
 {:file "form-init1493151512736136730.clj",
  :line 101,
  :var-scope user/eval23284}}

*Proposed: Add an extra key clojure.spec.alpha/fn that has the symbol of the var under instrumentation.

After:

user> (ex-data *e)
=>
{:clojure.spec.alpha/problems [{:path [:args :x],
                                :pred clojure.core/number?,
                                :val "x",
                                :via [],
                                :in [0]}],
 :clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__1210
                                  0x4db4a004
                                  "clojure.spec.alpha$regex_spec_impl$reify__1210@4db4a004"],
 :clojure.spec.alpha/value ("x"),
 :clojure.spec.alpha/fn user/foo,
 :clojure.spec.alpha/args ("x"),
 :clojure.spec.alpha/failure :instrument,
 :clojure.spec.test.alpha/caller {:file "form-init2105027941758372775.clj",
                                  :line 1,
                                  :var-scope user/eval1050}}

Patch: clj2166.patch

Screened by: Alex Miller



 Comments   
Comment by Josh Jones [ 23/Jun/17 3:58 PM ]

I've done a small patch to add this, but is there a consensus on whether the value for the newly-added key should be the var itself (referenced by a :clojure.spec.alpha/var key), or the symbol (referenced by a :clojure.spec.alpha/fn key)?

Comment by Alex Miller [ 23/Jun/17 4:29 PM ]

Symbol

Comment by Josh Jones [ 23/Jun/17 6:22 PM ]

I've attached a one-liner patch, and welcome any comments or issues.

Note that although I signed the CA today (Rich also signed) and can thus officially submit, I am not yet on "Signed Contributor Agreement" list at https://clojure.org/community/contributors . I can't edit posts on Jira yet, which I why I did not edit to mark that a patch is present.

I've read the contrib guide but this is my first clojure patch, so please bear with me if I have made any errors in the workflow or otherwise here. Thanks!

Using the example in the original ticket, the new output of ex-data is:

user> (ex-data *e)
=>
{:clojure.spec.alpha/problems [{:path [:args :x],
                                :pred clojure.core/number?,
                                :val "x",
                                :via [],
                                :in [0]}],
 :clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__1210
                                  0x4db4a004
                                  "clojure.spec.alpha$regex_spec_impl$reify__1210@4db4a004"],
 :clojure.spec.alpha/value ("x"),
 :clojure.spec.alpha/fn user/foo,
 :clojure.spec.alpha/args ("x"),
 :clojure.spec.alpha/failure :instrument,
 :clojure.spec.test.alpha/caller {:file "form-init2105027941758372775.clj",
                                  :line 1,
                                  :var-scope user/eval1050}}
Comment by Alex Miller [ 24/Jun/17 7:29 AM ]

Thanks, looks good. I've also added the groups for you to have edit rights.





[CLJ-2165] #clojure/var tag for transmitting var identity Created: 22/May/17  Updated: 11/Jul/17

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

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

Attachments: Text File vartag2.patch    
Approval: Vetted

 Description   

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

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

Patch: vartag2.patch



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

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





[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-2163] Add test for var serialization Created: 19/May/17  Updated: 20/Jun/17

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

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

Attachments: Text File var-test.patch    
Patch: Code and Test
Approval: Screened

 Description   

Add some tests for var serialization.

Screened by: Chouser






[CLJ-2162] condp should accept macros as first argument. Also would allow proper inlining Created: 07/May/17  Updated: 15/May/17

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

Type: Feature Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Currently condp takes the value of the first argument pred. This has two disadvantages:

  1. It won't allow macros to be used as the first argument
  2. It will be less efficient since the function calls can't be inlined anymore (common idiom: condp = x ...)

If pred is a symbol the let binding can be avoided. This would properly inline = which is common.



 Comments   
Comment by Alex Miller [ 07/May/17 3:30 PM ]

Concrete examples would help.

Comment by A. R [ 08/May/17 12:36 AM ]

In my example I have a macro that matches routes. Since they're static this can be optimized during compile time:

(condp route-match? x [:comments :view] 1 [:posts :edit] 2 [_ :delete] 3)

Would also provide a faster assoc for `records` which generates a `condp` with `identical`.





[CLJ-2161] clojure.spec.alpha fails to reload Created: 02/May/17  Updated: 03/May/17  Resolved: 02/May/17

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

Type: Defect Priority: Critical
Reporter: Jeaye Wilkerson Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: spec

Approval: Ok

 Description   

While looking to upgrade Orchestra to 1.9.0-alpha16, I found that I'm not able to reload clojure.spec.alpha more than once in the REPL. I've made a bare bones test case, linked below, which demonstrates the issue. I believe this is related to https://dev.clojure.org/jira/browse/CLJ-2098 but unrelated to autodoc, so I opted for a new ticket. That ticket is marked Critical, so I marked this similarly.

https://github.com/jeaye/clojure-spec-alpha-reload

The relevant Orchestra discussion is here: https://github.com/jeaye/orchestra/pull/3



 Comments   
Comment by Alex Miller [ 02/May/17 9:36 PM ]

Both of these problems are related to the current spec.alpha build not being AOT compiled. I've already made this change in spec.alpha for the next release and I believe that will fix the problem.

I've released a new version of spec.alpha (0.1.108) and I think if you update to that version, it should address the problem.

Comment by Daniel Compton [ 02/May/17 9:57 PM ]

Thanks for the quick turnaround, that version does seem to fix the problem.

Comment by Jeaye Wilkerson [ 03/May/17 12:06 AM ]

Thank you!





[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-2159] Disambiguate behavior of def with doc-string Created: 23/Apr/17  Updated: 23/Apr/17

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

Type: Enhancement Priority: Trivial
Reporter: Christopher Brown Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, documentation
Environment:

REPL


Attachments: Text File clarify-def-forms.patch    
Patch: Code

 Description   

As far as I can tell, it's impossible to use `def` to create a var that's unbound but has `:doc` metadata (or to change the `:doc` metadata of an existing var without also binding / changing the bound value).

This change clarifies the possible usages of `def`; i.e., if you supply `doc-string`, you must also supply `init`.






[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-2157] multi-spec doesn't generate possible tags from hierarchy Created: 22/Apr/17  Updated: 22/Apr/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: 0
Labels: generator, spec


 Description   

Problem: Even though multi-spec supports hierarchy dispatch of multi-methods, its generator only generates tags that have direct method implementations.

Proposed solution: It should also generate from hierarchy.






[CLJ-2156] Document char[] input support in clojure.java.io/copy Created: 22/Apr/17  Updated: 22/Apr/17

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

Type: Defect Priority: Trivial
Reporter: Yegor Timoshenko Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File copy-doc.patch    
Patch: Code
Approval: Prescreened

 Description   

https://github.com/clojure/clojure/blob/a26dfc1390c53ca10dba750b8d5e6b93e846c067/src/clj/clojure/java/io.clj#L393

copy actually supports char[] as input (see lines 373-380), so add to docstring.

Prescreened by: Alex Miller - tests already exist for this behavior, just not in docstring






[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: 1
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-2153] Documentation for int-in and int-in-range? does not mention that they're limited to fixed precision integers Created: 12/Apr/17  Updated: 26/May/17  Resolved: 26/May/17

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

Type: Enhancement Priority: Trivial
Reporter: Rovanion Luckey Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, spec

Attachments: Text File clj-2153-4.patch     Text File fixed-precision-doc-3.patch     Text File fixed-precision-doc.patch     Text File fixed-precision-doc.patch    
Patch: Code
Approval: Ok

 Description   

The documentation for clojure.spec/int-in and clojure.spec/int-in-range? does not explicitly mention that they only accept and produce fixed precision integers as opposed to all integer types including BigInt.

Patch: clj-2153-4.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 12/Apr/17 10:00 AM ]

To consider the patch, you need to sign the contributors agreement, which you can find here: https://clojure.org/community/contributing

Comment by Alex Miller [ 13/Apr/17 8:31 AM ]

Thanks for signing the CA. Please update the patch so it applies cleanly without whitespace warnings:

$ git apply ~/Downloads/fixed-precision-doc.patch
/Users/alex/Downloads/fixed-precision-doc.patch:32: trailing whitespace.
  "Return true if start <= val, val < end and val is a fixed
/Users/alex/Downloads/fixed-precision-doc.patch:40: trailing whitespace.
  "Returns a spec that validates fixed precision integers in the
warning: 2 lines add whitespace errors.

While this is admittedly not consistent within this file, I think it's best if the docstring lines are indented (with 2 spaces) after the first line, and that's what I'd like to see in the patch. Text changes are fine.

Comment by Rovanion Luckey [ 13/Apr/17 8:40 AM ]

Docstring now indented with two spaces after the first line.

Comment by Rovanion Luckey [ 13/Apr/17 8:42 AM ]

Now with even less trailing whitespaces.

Comment by Alex Miller [ 10/May/17 12:21 PM ]

Updated patch to apply to spec.alpha, no other changes, attribution retained.





[CLJ-2152] clojure.spec: s/& has a broken form Created: 12/Apr/17  Updated: 12/Apr/17

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

Type: Defect Priority: Major
Reporter: Tommi Reiman Assignee: Alex Miller
Resolution: Unresolved Votes: 3
Labels: spec
Environment:

1.9-alpha15


Approval: Vetted

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

(s/form (s/& integer?))
; (clojure.spec/& #object[clojure.core$integer_QMARK_ 0x5536db54 "clojure.core$integer_QMARK_@5536db54"])





[CLJ-2151] clojure.spec: s/merge reports errors multiple times with qualified keywords Created: 11/Apr/17  Updated: 11/Apr/17  Resolved: 11/Apr/17

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

Type: Defect Priority: Major
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: spec
Environment:

1.9-alpha15



 Description   

With unqualified keys, the problems look as expected. With qualified keys, the problems are shown from all branches?

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

(s/def ::a int?)
(s/def ::b string?)

(s/explain-data
  (s/merge
    (s/keys :req-un [::a])
    (s/keys :req-un [::b]))
  {:a 1 :b 1})
; #:clojure.spec{:problems ({:path [:b], :pred string?, :val 1, :via [:user/b], :in [:b]})}

(s/explain-data
  (s/merge
    (s/keys :req [::a])
    (s/keys :req [::b]))
  {::a 1 ::b 1})
;#:clojure.spec{:problems ({:path [:user/b], :pred string?, :val 1, :via [:user/b], :in [:user/b]}
;                          {:path [:user/b], :pred string?, :val 1, :via [:user/b], :in [:user/b]})}


 Comments   
Comment by Alex Miller [ 11/Apr/17 3:42 PM ]

Spec explain reports every error it found - in this case it found that the required key ::b is missing in both branches of the s/merge. So I think this is the expected behavior.





[CLJ-2150] c.l Created: 11/Apr/17  Updated: 11/Apr/17  Resolved: 11/Apr/17

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

Type: Defect Priority: Major
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Comments   
Comment by Tommi Reiman [ 11/Apr/17 7:35 AM ]

keybord typing error created multiple issues. Closing the unfinished duplicates.





[CLJ-2149] c.l Created: 11/Apr/17  Updated: 11/Apr/17  Resolved: 11/Apr/17

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

Type: Defect Priority: Major
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Comments   
Comment by Tommi Reiman [ 11/Apr/17 7:35 AM ]

keybord typing error created multiple issues. Closing the unfinished duplicates.





[CLJ-2148] c.l Created: 11/Apr/17  Updated: 11/Apr/17  Resolved: 11/Apr/17

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

Type: Defect Priority: Major
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Comments   
Comment by Tommi Reiman [ 11/Apr/17 7:35 AM ]

keybord typing error created multiple issues. Closing the unfinished duplicates.





[CLJ-2147] clojure.spec: s/keys* doesn't return a spec Created: 11/Apr/17  Updated: 11/Apr/17  Resolved: 11/Apr/17

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

Type: Defect Priority: Major
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: spec
Environment:

1.9.0-alpha15



 Description   
(s/spec? (s/keys* :req-un [::a ::b]))
; nil


 Comments   
Comment by Alex Miller [ 11/Apr/17 3:46 PM ]

This is pretty subtle but the regex ops return something that is a `regex?` but not a `spec?`. The reason for this has to do with being able to merge things that are regexes, but not merge across specs. (There is also some further history based in prior implementations.) In any case, this is currently the expected result.





[CLJ-2146] partition-by and partition-all transducers should ensure visibility of state changes Created: 09/Apr/17  Updated: 17/Apr/17

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: transducers

Approval: Vetted

 Description   

The partition-by and partition-all transducers use state stored in an ArrayList. This state should be protected (for example, by volatile) to ensure visibility if used in a transducing process that moves computations across threads.



 Comments   
Comment by Léo NOEL [ 13/Apr/17 1:00 PM ]

Discussion here : https://groups.google.com/forum/m/#!topic/clojure/VQj0E9TJWYY

Comment by Léo NOEL [ 16/Apr/17 9:47 AM ]

Note that following this logic, transients are as much broken, as they make use of plain arrays.
This paragraph https://clojure.org/reference/transients#_concurrent_use makes me believe this very problem has already been tackled before. Is the discussion available somewhere ?
In my opinion, documentation should be more precise about what is meant by thread isolation, and explain why it is OK to use unsynchronized mutable objects when they're owned by something that enforces sequential processing (agents, go blocks, channels, single-threading, etc).

Comment by Alex Miller [ 17/Apr/17 9:24 AM ]

Transients originally enforced thread isolation by recording and validating the originating thread. This was weakened to allow for transients passed around go blocks in Clojure 1.7 and has been through some rounds of fixes (like CLJ-1580). If there is an issue with them now, please file a separate ticket.





[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-2144] clojure.walk/keywordize-keys wants ns support for clojure.spec utility Created: 08/Apr/17  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: Aaron Brooks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-2144-Add-namespace-arg-to-walk-keywordize-keys.patch    

 Description   

keywordize-keys currently takes a single argument, a nested structure presumably containing maps, turning all string keys into un-namespaced keys. I've found that I've needed to maintain my own modified version of keywordize-keys that allows me to pass a namespace so I can import JSON objects and use them with clojure.spec (which strongly prefers namespaced keys).

The addition of an additional 2-arity invocation with a namespace string argument is a non-breaking change. I'll attach a patch once I have a JIRA number.



 Comments   
Comment by Aaron Brooks [ 08/Apr/17 1:51 PM ]

This patch also includes a test but I accidentally selected "Code" when I opened the ticket. I don't think I can change that. Can a maintainer update the "Patch" field to "Code and test"?

Comment by Alex Miller [ 11/Apr/17 4:10 PM ]

Another route to go with this btw is to first do CLJ-1899 to build on.





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

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

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

Approval: Vetted

 Description   

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

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


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

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





[CLJ-2142] Namespace map syntax prevents duplicate key check Created: 02/Apr/17  Updated: 26/May/17  Resolved: 26/May/17

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

Type: Defect Priority: Major
Reporter: Gregg Reynolds Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: reader

Attachments: Text File 0001-CLJ-2142-throw-on-duplicate-keys-in-namespaced-map-l.patch     Text File 0001-CLJ-2142-throw-on-duplicate-keys-in-namespaced-map-l-v2.patch     Text File clj-2142-3.patch    
Patch: Code and Test
Approval: Ok

 Description   
user> #::{:a 1 :a 2}
#:user{:a 2}

Cause: In the namespace map reader, a map is built by repeated assoc rather than via createWithCheck. Thus, assoc of same key replaces prior key rather than throwing an error.

Approach: Build an array and invoke RT.map(a), to echo same code path without namespace map literal syntax.

After:

user=> #::{:a 1 :a 2}
IllegalArgumentException Duplicate key: :user/a  clojure.lang.PersistentArrayMap.createWithCheck (PersistentArrayMap.java:71)

Patch: clj-2142-3.patch



 Comments   
Comment by Nicola Mometto [ 02/Apr/17 12:06 PM ]

updated patch also fixes EdnReader

Comment by Gregg Reynolds [ 02/Apr/17 1:23 PM ]

wow, that was fast, Nicola!





[CLJ-2141] New qualified-* predicates can return true, nil, and false Created: 31/Mar/17  Updated: 26/May/17  Resolved: 26/May/17

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

Type: Defect Priority: Major
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Completed Votes: 91
Labels: function

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

 Description   

As of Clojure 1.9-alpha15, the new qualifed-keyword?, qualified-symbol?, and qualified-ident? functions can return true, nil, or false depending on the input passed to them, e.g.:

=> (qualified-keyword? ::abc)
true
=> (qualified-keyword? :abc)
nil
=> (qualified-keyword? 'abc)
false

Returning nil rather than false for unqualified keywords has the following issues:

  • Many sources of Clojure documentation note that a trailing '?' in a Clojure function generally implies a true/false return so this violates a common expectation.
  • Returning true/false/nil complicates the ability of ClojureScript to optimize boolean returns. Theoretically, this might affect Clojure JIT optimization as well, turning a call taking the return value into polymorphic rather than bimorphic.
  • Returning true/false/nil can yield confusing results for functions that partition by value, like group-by

The prior implementation during development used `boolean` to coerce the return value to a null and that was removed for performance reasons (would require a call through another var).

Alternatives:

Perf benchmark (w/ criterium bench, Java 1.8, AOT, direct-linked)
Example: (bench (dotimes [_ 100] (qualified-keyword? :a)))

Impl :a :a/b note
(and (keyword? x) (namespace x) true) 96.749127 ns 96.412551 ns current impl (returns nil for :a)
(and (keyword? x) (some? (namespace x))) 99.014677 ns 96.149567 ns uses existing preds
(and (keyword? x) (not (nil? (namespace x)))) 97.512916 ns 95.008061 ns nil? inlines
(if (and (keyword? x) (namespace x)) true false) 98.617369 ns 97.866673 ns if-based
(if (keyword? x) (if (namespace x) true false) false) 99.000727 ns 99.474754 ns  

Note that these vary by less than 1 ns per invocation, so perf is not significantly different.

Approach: After discussion with Rich, have decided to use a boolean wrapper around the check: (boolean (and (keyword? x) (namespace x))).
Also, needed to move boolean implementation higher to colocate the qualified fns near the other fns.

Patch: clj-2141-3.patch



 Comments   
Comment by Didier A. [ 03/Apr/17 5:14 AM ]

Agreed, either make it truthy/falsey and drop the ?, or keep the convention, one of the fee ones that still does not have an exception to its rule.

Comment by Alex Miller [ 03/Apr/17 12:19 PM ]

Moving to vetted so we remember to look at this before 1.9 releases.

Comment by Nikita Prokopov [ 04/Apr/17 2:38 AM ]

Just wanted to add my 2cents:

  • Even if nobody ever promised explicitly that predicates strictly return true/false, it has become an established convention, people rely on it.
  • Performance benefits are debatable at best. The function itself might become slightly faster but the coercion to boolean is still happening at the call site (e.g. inside if/when condition).
  • I think problem is important enough that it must get at least a second look. If there’s no good reason to break the convention, it’ll be better for everybody if we keep following it.
  • Having predicates that return three possible values is, well, weird by all means.
  • Even worse is that they look exactly like the old predicates. There’s no way to tell. It adds nuance and complexity to otherwise pretty simple and straightforward part of Clojure

Sorry but I really think this was overlooked. I only wish best for Clojure ind its future. Please at least consider this.

Thanks!

Nikita.

Comment by Ertuğrul Çetin [ 04/Apr/17 6:25 AM ]

Totally agreed!

Comment by Murat Berk [ 05/Apr/17 9:38 AM ]

It should NOT violate a common expectation/convention.

Comment by Bozhidar Batsov [ 06/Apr/17 1:50 AM ]

Btw, this is also an opportunity to improve the docstrings of those functions. Apart from the fact that for the millionth time they're missing a final `.`, now we can add something like `otherwise it returns false` or something like this. I assume this was originally omitted due the possibility to return either nil or false.

Comment by Alex Miller [ 07/Apr/17 3:29 PM ]

Re the docstrings, no plans to update those. The "otherwise..." was omitted because it's omitted on all of the other predicate docstrings as well. That makes sense (if the common implication is that the return value is false).

Comment by Bozhidar Batsov [ 11/Apr/17 2:22 AM ]

Perhaps. On a more general note - I really think some docstring standard should be imposed at some point. I find it odd that some docstrings are terminated with `.`, some are not. Not to mention it's really hard to figure out in docstrings what's a reference to a parameter, what's a reference to some other var, etc. And inconsistent docstrings are hard to format/present - e.g. in Elisp and CL it's common to have the first line of the docstring as a separate sentence, so you could have a good one-line overview of the thing it describes. Anyways, that's completely off-topic. One day I might blog/speak about this.

Comment by Chris Oakman [ 26/May/17 1:12 PM ]

Just adding a note to say that I agree with Nikita Prokopov's comments above.

Comment by Alex Miller [ 26/May/17 3:28 PM ]

This change is included in 1.9.0-alpha17.





[CLJ-2140] surprising output from s/explain for a s/coll-of spec that encounters a map Created: 31/Mar/17  Updated: 31/Mar/17  Resolved: 31/Mar/17

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

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


 Description   

When a coll-of spec encounters a map, the explain output does not align with the validation issue:

(s/def :some/keywords (s/coll-of keyword?))
=> :some/keywords
(s/explain-data :some/keywords [:foo])
=> nil
(s/explain-data :some/keywords ["foo"])
=> #:clojure.spec{:problems ({:path [], :pred keyword?, :val "foo", :via [:some/keywords], :in [0]})}
(s/explain-data :some/keywords :foo)
=> #:clojure.spec{:problems [{:path [], :pred coll?, :val :foo, :via [:some/keywords], :in []}]}
(s/explain-data :some/keywords {:foo :bar})
=> #:clojure.spec{:problems ({:path [], :pred keyword?, :val [:foo :bar], :via [:some/keywords], :in [0]})}

I'd expect this from the last expression:

=> #:clojure.spec{:problems [{:path [], :pred coll?, :val {:foo :bar}, :via [:some/keywords], :in []}]}


 Comments   
Comment by Alex Miller [ 31/Mar/17 9:13 AM ]

Maps are collections and it is possible and occasionally useful to use s/coll-of or s-every to spec maps as collections of tuples. So, I think this is the expected and correct behavior.

There are some related cases in CLJ-2080 where this can go awry but the changes in the patch there would not affect this case.





[CLJ-2139] Couldn't satisfy such-that predicate after 100 tries error when using int? and int-in together Created: 29/Mar/17  Updated: 30/Mar/17  Resolved: 30/Mar/17

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

Type: Defect Priority: Minor
Reporter: Didier A. Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: newbie, spec
Environment:

Windows 7
Clojure 1.9.0-alpha15



 Description   

If you spec :args with (s/and int? (s/int-in x y)) you will get "ExceptionInfo Couldn't satisfy such-that predicate after 100 tries."

Here's code to reproduce:

Unable to find source-code formatter for language: clj. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(ns spec-test.core
  (:require [clojure.spec :as s]
            [clojure.spec.test :as st]))

(defn aaa [a] (.charAt "abcdef" a))
(s/fdef aaa
        :args (s/cat :a (s/and int? (s/int-in 0 5)))
        :ret any?)

(st/check `aaa)


 Comments   
Comment by Alex Miller [ 30/Mar/17 8:59 AM ]

The problem here is that when you use (s/and int? (s/int-in 0 5)) as the spec, the generator will generate random ints from the first subspec, then filter based on the second one. Because most of the numbers don't fall in the range, it will fail to generate. You can find more info about and generators at http://blog.cognitect.com/blog/2016/8/24/combining-specs-with-and.

In this particular case, it's easy to adjust this by simply reducing your spec to (s/int-in 0 5) which is designed to generate only values in that range.





[CLJ-2138] Can't use an aliased keyword in the same form as alias definition Created: 25/Mar/17  Updated: 26/Mar/17  Resolved: 26/Mar/17

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

Type: Defect Priority: Major
Reporter: Yegor Timoshenko Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

java version "1.8.0_111"
Java(TM) SE Runtime Environment (build 1.8.0_111-b14)
Java HotSpot(TM) 64-Bit Server VM (build 25.111-b14, mixed mode)



 Description   

user=> (require '[clojure.string :as str])
nil
user=> ::str/hello
:clojure.string/hello
user=> (do (require '[clojure.string :as str2]) ::str2/hello)

RuntimeException Invalid token: ::str2/hello clojure.lang.Util.runtimeException (Util.java:221)
RuntimeException Unmatched delimiter: ) clojure.lang.Util.runtimeException (Util.java:221)

At the same time, creating an alias and using a function from the aliased namespace is possible:

user=> (do (require '[clojure.string :as str3]) (str3/blank? ""))
true



 Comments   
Comment by Alex Miller [ 26/Mar/17 11:05 AM ]

Autoresolved keywords are a feature of the reader, which reads one form at a time. In this case, the whole do block is read prior to being evaluated so the alias context does not yet exist when that keyword is read.





[CLJ-2137] Clojure REPL doesn't ask for new input if line contains a keyword with a colon Created: 25/Mar/17  Updated: 26/Mar/17  Resolved: 26/Mar/17

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

Type: Defect Priority: Major
Reporter: Yegor Timoshenko Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: reader, repl
Environment:

java version "1.8.0_111"
Java(TM) SE Runtime Environment (build 1.8.0_111-b14)
Java HotSpot(TM) 64-Bit Server VM (build 25.111-b14, mixed mode)



 Description   
user=> :a
:a
user=> :a:z
:a:z
user=> [:a
  #_=> ]
[:a]
user=> [:a:z

RuntimeException EOF while reading, starting at line 1  clojure.lang.Util.runtimeException (Util.java:221)


 Comments   
Comment by Alex Miller [ 26/Mar/17 7:44 PM ]

This is not reproducible with the standard Clojure repl (invoking clojure.main). I can reproduce it with Leiningen so I'm guessing that's where you're seeing it. You can file a bug report for Leiningen at https://github.com/technomancy/leiningen.

Comment by Yegor Timoshenko [ 26/Mar/17 7:48 PM ]

Couldn't imagine that a build tool can affect the REPL. Thank you!





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

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

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

Tested against clojure 1.7 and 1.8



 Description   

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

Steps to reproduce:

File macro.clj:

(ns macro)

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

REPL:

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

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

PS. a workaround is to define the macro like

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


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

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





[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-2133] Clarify documentation for the satisfies? function. Created: 23/Mar/17  Updated: 23/Mar/17

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: David Chelimsky Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: docstring
Environment:

N/A



 Description   

The docs for satisfies? says "Returns true if x satisfies the protocol", but does not define the meaning of "satisfies". The function returns true when type and protocol are referenced in the same call to either extend-type or extend-protocol even when none of the protocol functions are implemented. I think the doc should be specific about this to avoid confusion.






[CLJ-2132] Maven pom requires artifact signing to install locally Created: 23/Mar/17  Updated: 20/Apr/17  Resolved: 20/Apr/17

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: build, regression

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

 Description   

The recent pom changes inadvertently made artifact signing a requirement for locally installing a Clojure build via:

mvn clean install

The attached patch moves the GPG plugin back into a profile (named "sign").






[CLJ-2131] partition-with Created: 19/Mar/17  Updated: 19/Mar/17  Resolved: 19/Mar/17

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

Type: Enhancement Priority: Trivial
Reporter: Derek Troy-West Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Any interest in introducing a partition fn that sits somewhere between partition-by and split-with?

(defn partition-with
"Applies f to each value in coll, splitting it each time f returns truthy
Returns a lazy seq of partitions."
[f coll]
(lazy-seq
(when-let [s (seq coll)]
(let [run (cons (first s) (take-while (complement f) (next s)))]
(cons run (partition-with f (seq (drop (count run) s))))))))

e.g

(partition-with #(= (rem % 3) 0) [1 2 3 6 7 8 9 12 13 15 16 17 18])
=> ((1 2) (3) (6 7 8) (9) (12 13) (15 16 17) (18))

I've used this occasionally and I notice it popped up on StackOverflow recently.

Not included thus far: the transducer arity or tests, but I'm happy to supply a patch if you're interested.



 Comments   
Comment by Derek Troy-West [ 19/Mar/17 6:31 AM ]

Apols for the formatting, I don't seem to be able to edit.

Comment by Derek Troy-West [ 19/Mar/17 7:12 AM ]

On reflection the special case of a seq of delimited sub-sequences is probably too narrow for core, which explains its current absence. Please ignore (I would kill the Jira myself, but..)

Comment by Alex Miller [ 19/Mar/17 12:32 PM ]

closed per request





[CLJ-2130] classof data-reader Created: 17/Mar/17  Updated: 17/Mar/17  Resolved: 17/Mar/17

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

Type: Enhancement Priority: Minor
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

I think it would be useful to have a #classof default data-reader which would read the supplied form as a Java class. Examples:

#classof String -> java.lang.String
#classof java.util.Map -> java.util.Map
#classof [String] -> [Ljava.lang.String;
#classof long -> long (aka Long/TYPE)
#classof [long] -> [J
#classof [[int]] -> [[I

So the idea is that a symbol would be read as the class to which it resolves (with support for primitives), and vectors would be read as the class of arrays of the classof the first (and only) item in the vector.

The main reason for wanting this is to have a readable form for array classes.



 Comments   
Comment by Alex Miller [ 17/Mar/17 4:02 PM ]

This is a solution, not a problem. The description only lightly mentions the problem at the end but does not demonstrate (preferably in an example) where this problem is encountered or why the current solution for these problems is not sufficient. The suggestion here is one idea, but no other alternatives are suggested, and tradeoffs are not considered.

Comment by Greg Chapman [ 17/Mar/17 8:01 PM ]

This is a fair criticism, and I regret having posted the idea without exploring it more fully. Especially as the above idea will not work for type-hinting (for some reason, I thought instances of Class could be used in :tags, but I now see that only Symbols and Strings are allowed).

Anyway, the thing that got me thinking along these lines is the annoyance involved with extending protocols to arrays. In particular, extend-type uses its t parameter in two ways: 1) emitted as-is to be evaluated (resolving to a Class) and passed as the first parameter to extend, and 2) in the unemitted metadata to type-hint the protocol functions. I don't think there's a way to satisfy both these uses with a single form representing an array class, so you have to fall back on extend and do your own type-hinting.

But that's not really that big a deal. I apologize for wasting your time with this.

Comment by Alex Miller [ 17/Mar/17 10:05 PM ]

You might also want to keep an eye on CLJ-1381.





[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-2128] spec error during macroexpand no longer throws compiler exception with location Created: 16/Mar/17  Updated: 26/May/17  Resolved: 26/May/17

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: error-reporting, regression, spec
Environment:

1.9.0-alpha12-1.9.0-alpha15


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

 Description   

This used to work but got out of sync with the commit https://github.com/clojure/clojure/commit/b3d3a5d6ff0a2f435bb6a5326da2b960038adad4, which changed the IllegalArgumentException to an ex-info, but didn't change the corresponding catch in the Compiler. That change is visible as of 1.9.0-alpha12.

Before alpha12:

...
Exception in thread "main" java.lang.IllegalArgumentException: Call to clojure.core/ns did not conform to spec:
In: [1] val: ((:required clojure.string)) fails at: [:args] predicate: (cat :docstring (? string?) :attr-map (? map?) :clauses :clojure.core.specs/ns-clauses),  Extra input
:clojure.spec/args  (th.core (:required clojure.string))
, compiling:(th/core.clj:1:1)
...

^^ note the "th/core.clj:1:1"

After alpha12:

...
Exception in thread "main" clojure.lang.ExceptionInfo: Call to clojure.core/ns did not conform to spec:
In: [1] val: ((:required clojure.string)) fails at: [:args] predicate: (cat :docstring (? string?) :attr-map (? map?) :clauses :clojure.core.specs/ns-clauses),  Extra input
:clojure.spec/args  (th.core (:required clojure.string))
 #:clojure.spec{:problems [{:path [:args], :reason "Extra input", :pred (cat :docstring (? string?) :attr-map (? map?) :clauses :clojure.core.specs/ns-clauses), :val ((:required clojure.string)), :via [], :in [1]}], :args (th.core (:required clojure.string))}, compiling:(/private/var/folders/7r/_1fj0f517rgcxwx79mn79mfc0000gn/T/form-init4120559363887828149.clj:1:125)
	at clojure.lang.Compiler.load(Compiler.java:7441)
...

Patch: clj-2128.patch






[CLJ-2127] clojure.spec/def requires literal keyword as first argument Created: 15/Mar/17  Updated: 15/Mar/17  Resolved: 15/Mar/17

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

Type: Defect Priority: Minor
Reporter: Anders Hessellund Jensen Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

I expected the following to work:
(clojure.spec/def (keyword "foo" "bar") string?)

It fails, because the def macro does not evaluate the first argument. Is this intentional? If so, perhaps the documentation or error message could be updated to reflect that the macro only accepts literal keywords.

As an aside, as a clojure beginner I found the semantics of clojure.spec/def confusing. The regular def macro and its variants creates bindings in the current namespace. clojure.spec/def does not modify the namespace, it registers specs in the spec registry under the provided key, even though it also accepts a symbol as an argument. I would have been less confused if the function was named clojure.spec/register!, the first argument was evaluated, and registering a symbol would require quoting of the symbol. That would make it behave like a regular function where arguments are evaluated as usual.



 Comments   
Comment by Alex Miller [ 15/Mar/17 9:37 AM ]

The docstring starts "Given a namespace-qualified keyword or resolvable symbol k" which seems to say the accurate thing. The error seems pretty clear to me too:

user=> (clojure.spec/def (keyword "foo" "bar") string?)
AssertionError Assert failed: k must be namespaced keyword or resolvable symbol
(c/and (ident? k) (namespace k))

If you really need to do something like this, it's easy enough to wrap with another macro like:

user=> (defmacro mydef [ns n s] `(s/def ~(keyword ns n) ~s))
#'user/mydef
user=> (mydef "foo" "bar" string?)
:foo/bar

Rich considered all the ramifications of the name when he named it, so that's not likely to change.

Comment by Anders Hessellund Jensen [ 15/Mar/17 10:49 AM ]

I still fail to see how I can infer from the documentation that k has to be a literal qualified keyword, and that an expression evaluating to a qualified keyword is not accepted. I assume it is my lack of Clojure experience.

Thanks for your response, and apologise for wasting your time.

Comment by Alex Miller [ 15/Mar/17 11:18 AM ]

This is the situation with many core macros (which are after all taking code and returning code and thus are a bit more literal-minded than functions). No apologies necessary - while I'm declining here, it's still useful feedback and might affect decisions in the future.





[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-2125] fspec doesn't generate pure functions. Created: 12/Mar/17  Updated: 12/Mar/17  Resolved: 12/Mar/17

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

Type: Enhancement Priority: Minor
Reporter: N/A Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: spec


 Description   

fspec doesn't generate pure functions.

(defn foo
[f]
(let [mf (memoize f)]
(= (mf 0) (mf 0))))

(s/fdef foo
:args (s/cat :f (s/fspec :args (s/cat :y int?)
:ret int?))
:ret true?)

(stest/check `foo)

=>
({:spec #object[clojure.spec$fspec_impl$reify__14282
0x20877912
"clojure.spec$fspec_impl$reify__14282@20877912"],
:clojure.spec.test.check/ret {:result true,
:num-tests 1000,
:seed 1489361298159},
:sym spike-spec.core/foo})

(defn bar
[f]
(= (f 0) (f 0)))

(s/fdef bar
:args (s/cat :f (s/fspec :args (s/cat :y int?)
:ret int?))
:ret true?)

(stest/check `bar)

=>
({:spec #object[clojure.spec$fspec_impl$reify__14282
0x56f3d7c7
"clojure.spec$fspec_impl$reify__14282@56f3d7c7"],
:clojure.spec.test.check/ret {:result #error{:cause "Specification-based check failed",
:data {:clojure.spec/problems [{:path [:ret],
:pred true?,
:val false,
:via [],
:in []}],
:clojure.spec.test/args (#object[clojure.spec$fspec_impl$reify_14282$fn_14285
0x7cf479f8
"clojure.spec$fspec_impl$reify_14282$fn_14285@7cf479f8"]),
:clojure.spec.test/val false,
:clojure.spec/failure :check-failed},
:via [{:type clojure.lang.ExceptionInfo,
:message "Specification-based check failed",
:data {:clojure.spec/problems [{:path [:ret],
:pred true?,
:val false,
:via [],
:in []}],
:clojure.spec.test/args (#object[clojure.spec$fspec_impl$reify_14282$fn_14285
0x7cf479f8
"clojure.spec$fspec_impl$reify_14282$fn_14285@7cf479f8"]),
:clojure.spec.test/val false,
:clojure.spec/failure :check-failed},
:at [clojure.core$ex_info
invokeStatic
"core.clj"
4725]}],
:trace [[clojure.core$ex_info
invokeStatic
"core.clj"
4725]
[clojure.spec.test$explain_check
invokeStatic
"test.clj"
279]
[clojure.spec.test$check_call
invokeStatic
"test.clj"
295]
[clojure.spec.test$quick_check$fn__13414
invoke
"test.clj"
308]
[clojure.lang.AFn
applyToHelper
"AFn.java"
154]
[clojure.lang.AFn
applyTo
"AFn.java"
144]
[clojure.core$apply
invokeStatic
"core.clj"
657]
[clojure.core$apply
invoke
"core.clj"
652]
[clojure.test.check.properties$apply_gen$fn_1500$fn_1501
invoke
"properties.cljc"
16]
[clojure.test.check.properties$apply_gen$fn__1500
invoke
"properties.cljc"
16]
[clojure.test.check.rose_tree$fmap
invokeStatic
"rose_tree.cljc"
78]
[clojure.test.check.rose_tree$fmap
invoke
"rose_tree.cljc"
74]
[clojure.test.check.generators$fmap$fn__629
invoke
"generators.cljc"
89]
[clojure.test.check.generators$gen_fmap$fn__579
invoke
"generators.cljc"
55]
[clojure.test.check.generators$call_gen
invokeStatic
"generators.cljc"
41]
[clojure.test.check.generators$call_gen
invoke
"generators.cljc"
38]
[clojure.test.check$quick_check
invokeStatic
"check.cljc"
62]
[clojure.test.check$quick_check
doInvoke
"check.cljc"
37]
[clojure.lang.RestFn
invoke
"RestFn.java"
425]
[clojure.lang.AFn
applyToHelper
"AFn.java"
156]
[clojure.lang.RestFn
applyTo
"RestFn.java"
132]
[clojure.core$apply
invokeStatic
"core.clj"
657]
[clojure.spec.gen$quick_check
invokeStatic
"gen.clj"
27]
[clojure.spec.gen$quick_check
doInvoke
"gen.clj"
27]
[clojure.lang.RestFn
applyTo
"RestFn.java"
137]
[clojure.core$apply
invokeStatic
"core.clj"
661]
[clojure.spec.test$quick_check
invokeStatic
"test.clj"
306]
[clojure.spec.test$check_1
invokeStatic
"test.clj"
334]
[clojure.spec.test$check$fn__13433
invoke
"test.clj"
410]
[clojure.core$pmap$fn_9385$fn_9386
invoke
"core.clj"
6897]
[clojure.core$binding_conveyor_fn$fn__6772
invoke
"core.clj"
2020]
[clojure.lang.AFn
call
"AFn.java"
18]
[java.util.concurrent.FutureTask
run
"FutureTask.java"
266]
[java.util.concurrent.ThreadPoolExecutor
runWorker
"ThreadPoolExecutor.java"
1142]
[java.util.concurrent.ThreadPoolExecutor$Worker
run
"ThreadPoolExecutor.java"
617]
[java.lang.Thread
run
"Thread.java"
745]]},
:seed 1489361392805,
:failing-size 0,
:num-tests 1,
:fail [(#object[clojure.spec$fspec_impl$reify_14282$fn_14285
0x7cf479f8
"clojure.spec$fspec_impl$reify_14282$fn_14285@7cf479f8"])],
:shrunk {:total-nodes-visited 0,
:depth 0,
:result #error{:cause "Specification-based check failed",
:data {:clojure.spec/problems [{:path [:ret],
:pred true?,
:val false,
:via [],
:in []}],
:clojure.spec.test/args (#object[clojure.spec$fspec_impl$reify_14282$fn_14285
0x7cf479f8
"clojure.spec$fspec_impl$reify_14282$fn_14285@7cf479f8"]),
:clojure.spec.test/val false,
:clojure.spec/failure :check-failed},
:via [{:type clojure.lang.ExceptionInfo,
:message "Specification-based check failed",
:data {:clojure.spec/problems [{:path [:ret],
:pred true?,
:val false,
:via [],
:in []}],
:clojure.spec.test/args (#object[clojure.spec$fspec_impl$reify_14282$fn_14285
0x7cf479f8
"clojure.spec$fspec_impl$reify_14282$fn_14285@7cf479f8"]),
:clojure.spec.test/val false,
:clojure.spec/failure :check-failed},
:at [clojure.core$ex_info
invokeStatic
"core.clj"
4725]}],
:trace [[clojure.core$ex_info
invokeStatic
"core.clj"
4725]
[clojure.spec.test$explain_check
invokeStatic
"test.clj"
279]
[clojure.spec.test$check_call
invokeStatic
"test.clj"
295]
[clojure.spec.test$quick_check$fn__13414
invoke
"test.clj"
308]
[clojure.lang.AFn
applyToHelper
"AFn.java"
154]
[clojure.lang.AFn
applyTo
"AFn.java"
144]
[clojure.core$apply
invokeStatic
"core.clj"
657]
[clojure.core$apply
invoke
"core.clj"
652]
[clojure.test.check.properties$apply_gen$fn_1500$fn_1501
invoke
"properties.cljc"
16]
[clojure.test.check.properties$apply_gen$fn__1500
invoke
"properties.cljc"
16]
[clojure.test.check.rose_tree$fmap
invokeStatic
"rose_tree.cljc"
78]
[clojure.test.check.rose_tree$fmap
invoke
"rose_tree.cljc"
74]
[clojure.test.check.generators$fmap$fn__629
invoke
"generators.cljc"
89]
[clojure.test.check.generators$gen_fmap$fn__579
invoke
"generators.cljc"
55]
[clojure.test.check.generators$call_gen
invokeStatic
"generators.cljc"
41]
[clojure.test.check.generators$call_gen
invoke
"generators.cljc"
38]
[clojure.test.check$quick_check
invokeStatic
"check.cljc"
62]
[clojure.test.check$quick_check
doInvoke
"check.cljc"
37]
[clojure.lang.RestFn
invoke
"RestFn.java"
425]
[clojure.lang.AFn
applyToHelper
"AFn.java"
156]
[clojure.lang.RestFn
applyTo
"RestFn.java"
132]
[clojure.core$apply
invokeStatic
"core.clj"
657]
[clojure.spec.gen$quick_check
invokeStatic
"gen.clj"
27]
[clojure.spec.gen$quick_check
doInvoke
"gen.clj"
27]
[clojure.lang.RestFn
applyTo
"RestFn.java"
137]
[clojure.core$apply
invokeStatic
"core.clj"
661]
[clojure.spec.test$quick_check
invokeStatic
"test.clj"
306]
[clojure.spec.test$check_1
invokeStatic
"test.clj"
334]
[clojure.spec.test$check$fn__13433
invoke
"test.clj"
410]
[clojure.core$pmap$fn_9385$fn_9386
invoke
"core.clj"
6897]
[clojure.core$binding_conveyor_fn$fn__6772
invoke
"core.clj"
2020]
[clojure.lang.AFn
call
"AFn.java"
18]
[java.util.concurrent.FutureTask
run
"FutureTask.java"
266]
[java.util.concurrent.ThreadPoolExecutor
runWorker
"ThreadPoolExecutor.java"
1142]
[java.util.concurrent.ThreadPoolExecutor$Worker
run
"ThreadPoolExecutor.java"
617]
[java.lang.Thread
run
"Thread.java"
745]]},
:smallest [(#object[clojure.spec$fspec_impl$reify_14282$fn_14285
0x7cf479f8
"clojure.spec$fspec_impl$reify_14282$fn_14285@7cf479f8"])]}},
:sym spike-spec.core/bar,
:failure #error{:cause "Specification-based check failed",
:data {:clojure.spec/problems [{:path [:ret],
:pred true?,
:val false,
:via [],
:in []}],
:clojure.spec.test/args (#object[clojure.spec$fspec_impl$reify_14282$fn_14285
0x7cf479f8
"clojure.spec$fspec_impl$reify_14282$fn_14285@7cf479f8"]),
:clojure.spec.test/val false,
:clojure.spec/failure :check-failed},
:via [{:type clojure.lang.ExceptionInfo,
:message "Specification-based check failed",
:data {:clojure.spec/problems [{:path [:ret],
:pred true?,
:val false,
:via [],
:in []}],
:clojure.spec.test/args (#object[clojure.spec$fspec_impl$reify_14282$fn_14285
0x7cf479f8
"clojure.spec$fspec_impl$reify_14282$fn_14285@7cf479f8"]),
:clojure.spec.test/val false,
:clojure.spec/failure :check-failed},
:at [clojure.core$ex_info invokeStatic "core.clj" 4725]}],
:trace [[clojure.core$ex_info invokeStatic "core.clj" 4725]
[clojure.spec.test$explain_check
invokeStatic
"test.clj"
279]
[clojure.spec.test$check_call
invokeStatic
"test.clj"
295]
[clojure.spec.test$quick_check$fn__13414
invoke
"test.clj"
308]
[clojure.lang.AFn applyToHelper "AFn.java" 154]
[clojure.lang.AFn applyTo "AFn.java" 144]
[clojure.core$apply invokeStatic "core.clj" 657]
[clojure.core$apply invoke "core.clj" 652]
[clojure.test.check.properties$apply_gen$fn_1500$fn_1501
invoke
"properties.cljc"
16]
[clojure.test.check.properties$apply_gen$fn__1500
invoke
"properties.cljc"
16]
[clojure.test.check.rose_tree$fmap
invokeStatic
"rose_tree.cljc"
78]
[clojure.test.check.rose_tree$fmap
invoke
"rose_tree.cljc"
74]
[clojure.test.check.generators$fmap$fn__629
invoke
"generators.cljc"
89]
[clojure.test.check.generators$gen_fmap$fn__579
invoke
"generators.cljc"
55]
[clojure.test.check.generators$call_gen
invokeStatic
"generators.cljc"
41]
[clojure.test.check.generators$call_gen
invoke
"generators.cljc"
38]
[clojure.test.check$quick_check
invokeStatic
"check.cljc"
62]
[clojure.test.check$quick_check
doInvoke
"check.cljc"
37]
[clojure.lang.RestFn invoke "RestFn.java" 425]
[clojure.lang.AFn applyToHelper "AFn.java" 156]
[clojure.lang.RestFn applyTo "RestFn.java" 132]
[clojure.core$apply invokeStatic "core.clj" 657]
[clojure.spec.gen$quick_check
invokeStatic
"gen.clj"
27]
[clojure.spec.gen$quick_check doInvoke "gen.clj" 27]
[clojure.lang.RestFn applyTo "RestFn.java" 137]
[clojure.core$apply invokeStatic "core.clj" 661]
[clojure.spec.test$quick_check
invokeStatic
"test.clj"
306]
[clojure.spec.test$check_1
invokeStatic
"test.clj"
334]
[clojure.spec.test$check$fn__13433
invoke
"test.clj"
410]
[clojure.core$pmap$fn_9385$fn_9386
invoke
"core.clj"
6897]
[clojure.core$binding_conveyor_fn$fn__6772
invoke
"core.clj"
2020]
[clojure.lang.AFn call "AFn.java" 18]
[java.util.concurrent.FutureTask
run
"FutureTask.java"
266]
[java.util.concurrent.ThreadPoolExecutor
runWorker
"ThreadPoolExecutor.java"
1142]
[java.util.concurrent.ThreadPoolExecutor$Worker
run
"ThreadPoolExecutor.java"
617]
[java.lang.Thread run "Thread.java" 745]]}})



 Comments   
Comment by Alex Miller [ 12/Mar/17 7:22 PM ]

The generated function uses the ret spec to generate results so I don't this does not seem like a goal.





[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: 4
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-2123] Lighter-weight aliasing for keywords Created: 10/Mar/17  Updated: 15/May/17

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

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

Approval: Vetted

 Description   

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

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






[CLJ-2122] flatten docstring does not describe lazy result Created: 07/Mar/17  Updated: 07/Mar/17

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

Type: Enhancement Priority: Trivial
Reporter: Alan Thompson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring
Environment:

All


Approval: Triaged

 Description   

clojure.core/flatten uses tree-seq to return a lazy result. The lazy nature of the result is not described in the docstring.

Original docstring:
--------------------------------
"Takes any nested combination of sequential things (lists, vectors,
etc.) and returns their contents as a single, flat sequence.
(flatten nil) returns an empty sequence."

Proposed docstring:
--------------------------------
"Takes any nested combination of sequential things (lists, vectors,
etc.) and returns their contents as a single, flat lazy sequence.
(flatten nil) returns an empty sequence."



 Comments   
Comment by Alex Miller [ 07/Mar/17 6:28 PM ]

Seems reasonable.





[CLJ-2121] Parameter names in docstring for `pos?` Created: 04/Mar/17  Updated: 05/Mar/17  Resolved: 05/Mar/17

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

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

Attachments: Text File 0001-CLJ-2121-update-docstring-to-reflect-param-name.patch    
Patch: Code
Approval: Triaged

 Description   

The docstring for `pos?` is as follows:

Usage: (pos? x)
Returns true if num is greater than zero, else false

This should be either:

Usage: (pos? num)
Returns true if num is greater than zero, else false

or

Usage: (pos? x)
Returns true if x is greater than zero, else false


 Comments   
Comment by Marc O'Morain [ 04/Mar/17 8:50 AM ]

(`neg?` and `zero?` have the same issue)

Comment by Alex Miller [ 04/Mar/17 10:58 AM ]

Patch welcome - change should update docstring, not arg.

Comment by Erik Assum [ 04/Mar/17 2:32 PM ]

Duplicate of CLJ-1859?

Comment by Alex Miller [ 05/Mar/17 7:08 PM ]

Closed as duplicate





[CLJ-2120] (for) works in REPL, but not in a file Created: 28/Feb/17  Updated: 28/Feb/17  Resolved: 28/Feb/17

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

Type: Defect Priority: Major
Reporter: Vadim Liventsev Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

elemenary OS 0.4 (Ubuntu 16.04.2 LTS / Linux 4.4.0-64-generic)
Java HotSpot(TM) 64-Bit Server VM 1.8.0_121-b13
Loading clojure 1.8.0 with Leiningen



 Description   

I have the following file:

for.clj
(println "for.clj loaded")
(for [fruit ["apple" "orange" "watermelon"]] (println fruit))

REPL is running in the same folder.
Calling

(for [fruit ["apple" "orange" "watermelon"]] (println fruit))
yields the expected result:

apple
orange
watermelon
(nil nil nil)

Calling

(load "for")
yields:

for.clj loaded
nil

The same happens when the file is loaded by Leiningen. It is also not limited to just (println), I have a project with a bunch of (for), it works in REPL, but grinds to a halt when loaded from a file.



 Comments   
Comment by Alex Miller [ 28/Feb/17 7:12 AM ]

for is lazy and won't evaluate its body unless something uses the resulting lazy seq. In the repl, the printing will do so but when you load, nothing will be doing the forcing.

You can use (doall (for ...)) to force the lazy seq around the for to be evaluated. Also see dorun and run! for some other fns that are used to force side effects.





[CLJ-2119] clojure.core/extends? inconsistent (erroroneous) between 1.7, 1.8/1.9.0-alpha14 Created: 28/Feb/17  Updated: 01/Mar/17  Resolved: 28/Feb/17

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

Type: Defect Priority: Major
Reporter: Tom S Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug, extends?
Environment:

windows 7/ 10
java version "1.8.0_111"
Java(TM) SE Runtime Environment (build 1.8.0_111-b14)
Java HotSpot(TM) 64-Bit Server VM (build 25.111-b14, mixed mode)



 Description   

Up to clojure 1.7, clojure.core/extends? served as a predicate as an alternative to the much much slower clojure.core/satisfies? pred for relatively quick type-based operations. I recently migrated to 1.8, and testing showed this no longer holds; clojure.core/extends? appears to be unable to detect that an object actually extends a protocol.

Simple example (1.7):

;user=> (defn ikv? [obj] (extends? clojure.core.protocols/IKVReduce (type obj)))
;#'user/ikv?
;user=> (ikv? {:a 2})
;true

Called against a clojure.lang.PersistentArrayMap, which implements IKVReduce in its java class definition, this makes sense.

The same example reports false on clojure 1.8 (and 1.9.0-alpha14).
However, clojure.core/satisfies? is true in 1.8 (and 1.9.0-alpha14).

My hacked workaround is to use Alex Miller's example of memoizing the call to satisfies? (slightly slower than extends? but reasonable for my use cases.

However, the fundamental behavior of extends? seems to be broken, which may be a deeper issue going forward. Plus, it's a otherwise "silent" error, since it happily returns false, leading to silent, possibly obscure runtime errors.



 Comments   
Comment by Alex Miller [ 28/Feb/17 7:30 AM ]

I think you're relying on implementation details that are subject to change.

extends? can only catch protocol extensions that happen in the type definition, not ones that are applied later. So checking a protocol with satisfies? is the correct way to do it.

That said, I'll double check and make sure nothing is amiss.

Comment by Alex Miller [ 28/Feb/17 8:50 AM ]

Yeah, this change in what you're seeing is due to the refactoring in https://github.com/clojure/clojure/commit/684cd4117bb2cf463c95293855a0dff52134bdcd. Again, the fact that extends? worked before was relying on unreliable details of the specific way things are implemented and what you're really trying to check is something about the protocol, which requires satisfies?.

Comment by Tom S [ 01/Mar/17 4:11 AM ]

I completely missed the fact that clojure.lang.IKVReduce popped up during
the refactor, and is used as a fast-path for classes that implement it
during kv-reduce.

I chased my tail trying to figure out why maps don't extend
clojure.core.protocols/IKVReduce, which is simply because the interface
clojure.core.protocols.IKVReduce is no longer implemented, leaving
isAssignAbleFrom reflection call to return false now, while providing a
protocol implementation for clojure.lang.IKVReduce. The protocol
implementation delegates to the class's implementation of
clojure.lang.IKVReduce if possible.

So, satisfies? traces through superclasses trying to find a protocol
implementation, which is far more durable (depending on the protocol)
rather than class-specific interface implementations I was using to
cheat out a fastpath.

Just for grins, here's the microbenchmark associated with the "fast path"
vs cached method lookup:
(def the-map {:a 2})

;;note the clojure.lang.KVReduce
(defn ikv? [x] (instance? clojure.lang.KVReduce x))

(let [cache (java.util.HashMap.)]
(defn sat? [protocol x]
(let [c (class x)]
(if-let [res (.get cache c)] ;we know.
res
(let [res (satisfies? protocol x)
_ (.put cache c res)]
res)))))

;;Microbenchmarks.

;;(time (dotimes [i 1000000] (sat? clojure.core.protocols/IKVReduce the-map)))
;;"Elapsed time: 13.31948 msecs"

;;(time (dotimes [i 1000000] (ikv? the-map))
;;"Elapsed time: 6.816777 msecs"

Negligible cost for caching (preferably using a concurrent map in production).

Thanks for tracking down the refactoring change. Sorry for the false alarm.

Comment by Tom S [ 01/Mar/17 4:43 AM ]

;typo correction.
(defn ikv? [x] (instance? clojure.lang.IKVReduce x))





[CLJ-2118] extend-type doesn't identify that a protocol is a protocol Created: 23/Feb/17  Updated: 23/Feb/17  Resolved: 23/Feb/17

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

Type: Defect Priority: Major
Reporter: Maurício Eduardo Chicupo Szabo Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: protocols
Environment:

Ubuntu 16.04 x86_64, OpenJDK 8



 Description   

I'm trying to implement this tutorial: http://www.lucagrulla.com/posts/server-sent-events-with-ring-and-compojure/

In one part, I need to extend a core.async type with the following protocol: https://github.com/ring-clojure/ring/blob/master/ring-core/src/ring/core/protocols.clj#L19.

So, I've tried to implement something like this:

src/async_test/protocol_ext.clj
(ns async-test.protocol-ext
  (:require [clojure.core.async :as async]
            [clojure.java.io :as io])
  (:import [ring.core.protocols StreamableResponseBody]
           [clojure.core.async.impl.channels ManyToManyChannel]))

(extend-type ManyToManyChannel
  StreamableResponseBody
  (write-body-to-stream [channel response output-stream]
                        (async/go (with-open [writer (io/writer output-stream)]
                                    (async/loop []
                                                (when-let [msg (async/<! channel)]
                                                  (doto writer (.write msg) (.flush))
                                                  (recur)))))))

But this fails with interface ring.core.protocols.StreamableResponseBody is not a protocol.

Then, I tried to monkey-patch ring: opened a new file with the correct path, and added the following lines on the bottom of the protocol declaration, inside `extend-protocol` call:

src/ring/core/protocols.clj
ManyToManyChannel
  (write-body-to-stream [channel response output-stream]
                        (async/go (with-open [writer (io/writer output-stream)]
                                    (async/loop []
                                                (when-let [msg (async/<! channel)]
                                                  (doto writer (.write msg) (.flush))
                                                  (recur)))))))

Then, it worked. What's happening? Is Clojure ignoring that StreamableResponseBody is a protocol if there's already an `extend-protocol` call?



 Comments   
Comment by Maurício Eduardo Chicupo Szabo [ 23/Feb/17 3:00 PM ]

Okay, my bad. Seems that I need to `:require` the protocol, not `:import` it. Sorry about that...

Comment by Alex Miller [ 23/Feb/17 3:04 PM ]

The issue here is that you are importing StreamableResponseBody as a Java class (really an interface). While the protocol does generate a Java interface, you should use the protocol, not the interface, when you extend. This is confusing because they both have the same name.

So, your ns declaration should instead be:

(ns async-test.protocol-ext
  (:require [clojure.core.async :as async]
            [clojure.java.io :as io]
						[ring.core.protocols :refer [StreamableResponseBody]])
  (:import [clojure.core.async.impl.channels ManyToManyChannel]))

The monkeypatch you tried worked because you were properly referring to the protocol in that case.





[CLJ-2117] Support typical polymorphic maps in spec more directly Created: 23/Feb/17  Updated: 23/Feb/17  Resolved: 23/Feb/17

Status: Closed
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: Declined Votes: 0
Labels: spec


 Description   

Problem: A common scenario is that one wants to spec this:

(def a-poly-map
  {::type :t1
   <kvs required/optional in t1>
   <kvs required/optional in all>
   })

To do this, you usually write lots of boiler plate code like this:

(s/def ::type #{:t1 <more-types>})

(defmulti type-dispatch ::type)
(defmethod type-dispatch :t1 [_] (s/keys :req [<t1-keys>]))
<more defmethods for more-types>

(s/def ::poly-map (s/merge (s/keys :req [::type <poly-map-keys-in-all>])
                           (s/multi-spec type-dispatch ::type))

As a workaround one can (and we did) of course write a macro to write all that code.

From my experience this usecase is the 90% multi-spec usecase and thus I'd prefer if a more convenient to use spec existed.

Proposed solution: Implement this spec:

(s/def ::poly-map (s/merge (s/keys :req [<poly-map-keys-in-all>]
                           (s/poly-map ::type
                             :t1 {:req [<t1-keys>]} ; :req, :opt etc. as in s/keys
                             <more requirements for more types>)))

Limitations: This syntax does not support to reuse existing specs in for dispatch values (:t1, :t2, ...) intentionally, because it seems an non-existing usecase to spec incomplete s/keys for such maps separately anyway.

Further enhancement: While the <poly-map-keys-in-all> usecase is rare, this syntax could also directly support it:

(s/def ::poly-map (s/poly-map ::type
                    {:req [<poly-map-keys-in-all>]} ; optional
                    :t1 {:req [<t1-keys>]}
                    <more requirements for more types>))

This new spec would automatically check for the presence of ::type and conform/explain like a regular s/keys.



 Comments   
Comment by Alex Miller [ 23/Feb/17 11:50 AM ]

Thanks, but I don't think we expect to do anything like this.

Comment by Leon Grapenthin [ 23/Feb/17 12:03 PM ]

Why?

Comment by Alex Miller [ 23/Feb/17 12:51 PM ]

Because spec provides the tools to do it already.

Comment by Leon Grapenthin [ 23/Feb/17 1:55 PM ]

This ticket is asking for a syntactic simplification of a typical use case. I understand that there already is s/multi-spec. s/multi-spec however is a generic and in comparison quite advanced tool that is not tied to maps. Thus it requires a substantial amount of boilerplate to "do it".

After much practical experience with spec in my observation - and I do assume others would agree - this is the most common application for it.

There are many things in clojure.core which aren't the only "tools to do it" for the exact same reason, so please reconsider not closing this right away.

Comment by Alex Miller [ 23/Feb/17 2:32 PM ]

My job is to weigh the balance of several factors and decide if an enhancement request seems reasonably likely to remain as an open ticket and at some point be further evaluated.

Some factors I try to think about:

  • Is this a good problem? Does it describe a pain point that many people are likely to encounter? You claim it is "most common" use but seems way too early to judge this (from what I've seen, I wouldn't agree right now).
  • Is the pain significant enough that it prevents you from doing your work? In this case, clearly not - you've already built it and it would be easy to release it independently from core.
  • Is this a fundamental (simple) feature that belongs in core? Clojure has a small library and wishes to keep it that way. There are ample examples where this constraint was not considered enough in Clojure. From what I can tell, this is appears to be a composite of existing things, not a fundamental part.
  • If we were to solve this problem in core, is this the proposed solution a likely way we would do it? Based on things we've talked about during dev, my suspicion is, probably not.

On balance, it does not seem to be something worth leaving open in the tracker right now. But as the saying goes, "no is temporary, yes is forever". If a while down the road it's clear that this is a prominent problem that needs attention, there is nothing preventing us from considering it again (presumably with more data and experience at that time).

Comment by Leon Grapenthin [ 23/Feb/17 3:11 PM ]

Thanks for explaining the decision. I understand that you have priorities and probably a better plan to address this problem in the future.





[CLJ-2116] Support for selective conforming with clojure.spec Created: 22/Feb/17  Updated: 22/Jul/17

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

Type: Feature Priority: Major
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Unresolved Votes: 22
Labels: spec
Environment:

[org.clojure/clojure "1.9.0-alpha14"]


Attachments: Text File clj-2116.patch    

 Description   

Problem

using clojure.spec in runtime border validation supporting multiple exchange formats is hard.

Details

Currently in clojure.spec (alpha-14), conformers are attached to Spec instances at creation time and they are invoked on every conform. This is not very useful in system border validation, where conforming/coercion functions should be selected based on runtime data, e.g. the exchange format.

Examples:

  • a keyword? spec:
    • with EDN, no coercion should be done (it can present Keywords)
    • with JSON, String->Keyword coercion should be applied
    • with String-based formats (CSV, query-params, ...), String->Keyword coercion should be applied
  • a integer? spec:
    • with EDN, no coercion should be done (it can present numbers)
    • with JSON, no coercion should be done (it can present numbers)
    • with String-based formats (CSV, query-params, ...), String->Long coercion should be applied

Here is a more complete example:

(s/def ::id integer?)
(s/def ::name string?)
(s/def ::title keyword?)
(s/def ::person (s/keys :opt [::id], :req-un [::name ::title]))

;; this is how we see the data over different exchange formats
(def edn-person {::id 1, :name "Tiina", :title :boss})
(def json-person {::id 1, :name "Tiina", :title "boss"})
(def string-person {::id "1", :name "Tiina", :title "boss"})

;; here's what we want
(def conformed-person edn-person)

To use this today, one needs to manually create new border specs with different conformers for all different exchange formats. Non-qualified keywords could be mapped in s/keys to work (e.g. ::title => ::title$JSON), but this wont work if fully qualified keys are exposed over the border (like ::id in the example) - one can't register multiple, differently conforming version of the spec with same name.

Suggestion

Support selective conforming in the Spec Protocol with a new 3-arity conform* and clojure.spec/conform, both taking a extra user-provided callback/visitor function. If the callback is provided, it's called from within the Specs conform* with the current spec as argument and it will return either nil or a 2-arity conformer function that should be used for the actual confrom.

Actual conforming-matcher implementations can be maintained in 3rd party libraries, like spec-tools[1].

Using it would look like this:

;; edn
(assert (= conformed-person (s/conform ::person edn-person)))
(assert (= conformed-person (s/conform ::person edn-person nil)))

;; json
(assert (= conformed-person (s/conform ::person json-person json-conforming-matcher)))

;; string
(assert (= conformed-person (s/conform ::person string-person string-conforming-matcher)))

Alternative

Another option to support this would be to allow Specs to be extended with Protocols. 3rd party libs could have a new Conforming protocol with 3-arity conform and add implementations for it on all current specs. Currently this is not possible.

[1] https://github.com/metosin/spec-tools



 Comments   
Comment by Alex Miller [ 22/Feb/17 3:33 PM ]

I don't think we are interested in turning spec into a transformation engine via conformers, so I suspect we are probably not interested. However, I'll leave it for Rich to assess.

Comment by Tommi Reiman [ 23/Feb/17 1:26 AM ]

Currently, Plumatic Schema is the tool used at the borders. Now, people are starting to move to Spec and it would really bad for the Clojure Web Developement Story if one had to use two different modelling libraries for their apps. If Spec doesn't want to be a tranformation engine via conformers, I hope for the Alternative suggestion to allow 3rd parties to write this kind of extensions: exposing Specs as Records/Types instead of reified protocols would do the job?

Comment by ken restivo [ 28/Feb/17 9:43 PM ]

I could see why the Clojure core developers might not want Spec to support this kind of coercion, but the practical reality is that someone will have to. If it isn't in Spec itself, it'll have to be done libraries built upon it like Tommi's.

The use case here is: I have a conf file that is YAML. I'm parsing the YAML using a Clojure library, turning it into a map. Now I have to validate the map, but YAML doesn't support keywords, for example, and the settings structure goes directly into Component/Mount/etc as part of the app state, so it makes sense to run s/conform on it as the first step in app startup after reading configuration. Add to this the possibility of other methods of merging in configuration (env vars, .properties files, etc) and this coercion will be necessary somewhere.

Comment by Tommi Reiman [ 08/May/17 1:03 PM ]

Any news on assessing this? I would be happy to provide a patch or a link to a modified clojure.spec with samples on usage with the 3-arity conform in it. Some thinking aloud: http://www.metosin.fi/blog/clojure-spec-as-a-runtime-transformation-engine/

Comment by Alex Miller [ 09/May/17 10:10 AM ]

Rich hasn't looked at it yet. My guess is still that we're not interested in this change. While I think some interesting problems are described in the post, I don't agree with most of the approaches being taken there.

Comment by Simon Belak [ 10/May/17 4:39 AM ]

Why not just use s/or (or s/alt) and then dispatch on the tag. Something like:

(s/def ::id (s/and (s/or :int integer?
                         :str string?)
                   (s/conformer (fn [[tag x]]
                                  (case tag
                                    :int x
                                    :str (Integer/parseInt x))))))

I use that pattern quite a bit in https://github.com/sbelak/huri and with a bit of syntactic sugar it works quite well.

Comment by Imre Kószó [ 12/May/17 3:46 AM ]

Simon that will not work if you are trying to conform to specs from third parties though. One of the points of this suggestion is that third parties would be able to write their own conformers to existing specs without redefining those specs.

Comment by Tommi Reiman [ 08/Jun/17 1:40 AM ]

Thanks for the comments. I would be happy to provide a patch / sample repo with the changed needed for this, in hope that it would help to decide if this could end up in the spec or not. What do you think?

Below is a sample of initial spec-integration into ring/http libs, using spec-tools. For now, one needs to wrap specs into spec records to enable the 3-arity conforming. This is boilerplate I would like to see removed. With this change, it should work out-of-box for all (3rd party) specs.

(require '[compojure.api.sweet :refer :all])
(require '[clojure.spec.alpha :as s])
(require '[spec-tools.core :as st])

;; to enable 3-arity conforming
(defn enum [values]
  (st/spec (s/and (st/spec keyword?) values)))

(s/def ::id int?)
(s/def ::name string?)
(s/def ::description string?)
(s/def ::size (enum #{:L :M :S}))
(s/def ::country (st/spec keyword?) ;; to enable 3-arity conforming
(s/def ::city string?)
(s/def ::origin (s/keys :req-un [::country ::city]))
(s/def ::new-pizza (st/spec (s/keys :req-un [::name ::size ::origin] :opt-un [::description])))
(s/def ::pizza (st/spec (s/keys :req [::id] :req-un [::name ::size ::origin] :opt-un [::description])))

;; emits a ring-handler with input & output validation (& swagger-docs)
;; select conforming based on request content-type (e.g. json/edn) + strip-extra keys from maps
(context "/spec" []
  (resource
    {:coercion :spec
     :parameters {:body-params ::new-pizza}
     :responses {200 {:schema ::pizza}}
     :post {:handler (fn [{new-pizza :body-params}]
                       (ok (assoc new-pizza ::id 1))}}))
Comment by Tommi Reiman [ 21/Jul/17 4:13 AM ]

Intended to create internal PR in my fork of clojure.spec, but ended up doing a real DUMMY PR for the actual repo. Well, here it is anyway:

https://github.com/clojure/spec.alpha/pull/1

Happy to finalize & create a patch into Jira if this goes any further.

Comment by Tommi Reiman [ 21/Jul/17 4:14 AM ]

comments welcome. here's a sample test for it:

(deftest conforming-callback-test
  (let [string->int-conforming
        (fn [spec]
          (condp = spec
            int? (fn [_ x _]
                   (cond
                     (int? x) x
                     (string? x) (try
                                   (Long/parseLong x)
                                   (catch Exception _
                                     ::s/invalid))
                     :else ::s/invalid))
            :else nil))]

    (testing "no conforming callback"
      (is (= 1 (s/conform int? 1)))
      (is (= ::s/invalid (s/conform int? "1"))))

    (testing "with conforming callback"
      (is (= 1 (s/conform int? 1 string->int-conforming)))
      (is (= 1 (s/conform int? "1" string->int-conforming))))))
Comment by Tommi Reiman [ 22/Jul/17 2:12 AM ]

initial work as patch.





[CLJ-2115] Support data conveying conform errors (alternative/complement to :clojure.spec/invalid) Created: 22/Feb/17  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: Max Penet Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: spec


 Description   

At the moment if a conform calls fails (returning :clojure.spec/invalid) there is no way to supply extra information about why it failed. We do have the possibility to get explain-data, but at best this would return a spec form, the original value and some metadata.

While this is fine in most cases, some conformer functions upon failure can provide extra data that'd be useful to the consumer. A practical example we had was a spec that can contain values for a String based DSL (think SQL like), that would conform these values to their parsed AST. When the conform wrapped function fails it would throw an ex-info with line/col info and more metadata about the failure. But all this data was lost since we can only return :clojure.spec/invalid. All this happened inside a rule engine schema, that can contain hundreds of these; re-parsing all the failing values for error reporting is something we wanted to avoid.

The proposal would be to to support a new return value that'd allow conveying data about the conform failure, or to support both this new value and :clojure.spec/invalid.
This could take the form of (explain-info {..}) potentially returned by conformer function for later consumption by explain, to match clojure semantics with exceptions (ex-info/ex-data, explain-info/explain-data).

A more naive implementation could just allow to throw inside the conformer function and have the error merged/assoced into the explain map (but that might be a bit too invasive in my opinion).






[CLJ-2114] ::defn-args spec incorrectly parses map body as a prepost rather than function body Created: 16/Feb/17  Updated: 14/Mar/17  Resolved: 14/Mar/17

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: spec

Attachments: Text File 0001-CLJ-2144-conform-map-fn-bodies-as-body-rather-than-a.patch    
Patch: Code
Approval: Ok

 Description   

Reported by Claire Alvis in #clojure-spec:

user> (s/conform :clojure.core.specs/defn-args
                 '(foo [bar] {:baz 42}))
{:name foo, :bs [:arity-1 {:args {:args [[:sym bar]]}, :prepost {:baz 42}}]}

The current spec conforms function bodies with single maps as prepost conditions rather than function bodies, after the patch:

user=> (s/conform :clojure.core.specs/defn-args '(foo [bar] {:baz 42}))
{:name foo, :bs [:arity-1 {:args {:args [[:sym bar]]}, :body [:body [{:baz 42}]]}]}
user=> (s/conform :clojure.core.specs/defn-args '(foo [bar] {:baz 42} 1))
{:name foo, :bs [:arity-1 {:args {:args [[:sym bar]]}, :body [:prepost+body {:prepost {:baz 42}, :body [1]}]}]}

Patch: 0001-CLJ-2144-conform-map-fn-bodies-as-body-rather-than-a.patch

Screened by: Alex Miller



 Comments   
Comment by Nicola Mometto [ 16/Feb/17 5:42 PM ]

An open question is whether we also want to make `:prepost` stricter as part of this patch, so that it will ensure that `:pre` and `:post` are a collection





[CLJ-2113] Update Clojure maven for latest on CI server Created: 16/Feb/17  Updated: 14/Mar/17  Resolved: 14/Mar/17

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: build

Attachments: Text File mvn3.patch    
Patch: Code
Approval: Ok

 Description   

Update maven build infrastructure to stop using oss-parent and use updated Maven 3 and sonatype plugins (similar to other changes made recently in all contrib projects).

  • Removed oss-parent parent pom. This has been deprecated for years and is no longer recommended for use.
  • Add snapshot repo (was previously pulled in via oss-parent)
  • maven-compiler-plugin - update to latest version
  • maven-release-plugin - update to latest version
  • add nexus-staging-maven-plugin - current recommended plugin for releases to maven central, replaces most of the maven-release-plugin's work (old version of this previously in oss-parent)
  • add maven-gpg-plugin for signing (previously in oss-parent)
  • remove old release profile which was activated by oss-parent pom

Patch: mvn3.patch

It's difficult to test this completely outside the context of actually building and deploying snapshots and releases but the changes are very similar to those made for all contrib projects recently.






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

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

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

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

 Description   

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

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

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






[CLJ-2111] Clarify s/every docstring for :kind Created: 14/Feb/17  Updated: 28/Jun/17

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

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

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

 Description   

Docstring for `s/every` in the `:kind` option says "a pred/spec that the collection type must satisfy, e.g. vector?" but using a spec for `:kind` will fail:

user=> (s/valid? (s/every number? :kind (s/or :vector vector? :list list?))
          [])
ClassCastException clojure.spec$or_spec_impl$reify__13891 cannot be cast to clojure.lang.IFn  dev/eval44499/fn--44501 (form-init3178965928127409998.clj:22)

user=> (pst *e)
ClassCastException clojure.spec$or_spec_impl$reify__13903 cannot be cast to clojure.lang.IFn
	user/eval20/fn--22 (NO_SOURCE_FILE:13)
	clojure.spec/every-impl/reify--14039 (spec.clj:1225)
	clojure.spec/valid? (spec.clj:744)
	clojure.spec/valid? (spec.clj:740)

Proposed: The intent here was just to support a predicate function. Change docstring to say just "pred" rather than "pred/spec".

Patch: clj-2111.patch



 Comments   
Comment by Alex Miller [ 14/Feb/17 5:06 PM ]

Certainly a function like this works (s/every number? :kind #(or (vector? %) (list? %))). The question is whether the s/every doc that states "pred/spec" means only a predicate function or "predicate function OR spec". I'm not sure what the intention was. Certainly the code in every seems to be wrapping the kind into a function and then invoking it in every-impl, so it's not written to accept a spec currently.

Comment by Alex Miller [ 14/Feb/17 5:06 PM ]

Marking vetted to either resolve, update docstring, or decline. Need more info from Rich.





[CLJ-2110] sorted map returns nil value for existing key Created: 14/Feb/17  Updated: 14/Feb/17  Resolved: 14/Feb/17

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

Type: Defect Priority: Major
Reporter: jonnik Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: sorted-map
Environment:

Oracle Java 1.8.0_112



 Description   

Then i try to get value by key from sorted map with comparator by value it returns nil.

(def tmap {1 {:v 1} 2 {:v 2} 3 {:v 3}})
(def tmap-sorted (apply sorted-map-by #(let [val-comp (- (compare (get-in tmap [%1 :v]) (get-in tmap [%2 :v])))]
                                         (if (= val-comp 0) 1 val-comp))
                        (flatten (vec tmap))))
; => {3 {:v 3} 2 {:v 2} 1 {:v 1}}
(get tmap-sorted 3)
;=> nil

Expected: {:v 3}
Actual: nil



 Comments   
Comment by Andy Fingerhut [ 14/Feb/17 11:31 AM ]

Your comparison function never returns 0, by the way you have written it. If it never returns 0, it will never 'think' that it has found an equal element when searching for a match using 'get'. By replacing (if (= val-comp 0) 1 val-comp) with val-comp, the get calls will work:

(def tmap-sorted (apply sorted-map-by #(let [val-comp (- (compare (get-in tmap [%1 :v]) (get-in tmap [%2 :v])))]
                                         val-comp)
                        (flatten (vec tmap))))
tmap-sorted
; => {3 {:v 3}, 2 {:v 2}, 1 {:v 1}}
(get tmap-sorted 3)
; => {:v 3}

You are getting a descending sorted order by negating the return value of compare. I would recommend to follow the advice on this page: https://clojure.org/guides/comparators particularly "Reverse the sort by reversing the order that you give the arguments to an existing comparator." That helps avoid corner cases with some integer values.

I would also recommend (into my-empty-sorted-map tmap) in place of your (apply my-empty-sorted-map (flatten (vec tmap)). Putting all of those recommendations together would result in code like this:

(def tmap-sorted2 (into (sorted-map-by #(compare (get-in tmap [%2 :v]) (get-in tmap [%1 :v])))
                        tmap))
tmap-sorted2
; => {3 {:v 3}, 2 {:v 2}, 1 {:v 1}}
(get tmap-sorted2 3)
; => {:v 3}
Comment by saintech [ 14/Feb/17 11:41 AM ]

Ok. But how about this?:

tmap-sorted
; => {3 {:v 3} 2 {:v 2} 1 {:v 1}}
(first tmap-sorted)
; => [3 {:v 3}]
(get tmap-sorted 3)
;=> nil

Is it OK?

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

I believe that calling clojure.core/first on a sorted-map does not cause its comparison function to be called at all. It is already stored in a sorted tree in memory, and first just finds the earliest one.

clojure.core/get does call the comparison function, perhaps several times, to find an item with an equal key. The original comparison function given in the description never returns equality (i.e. the integer 0) when comparing two items.

Comment by Alex Miller [ 14/Feb/17 1:32 PM ]

Agreed with Andy, declining





[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-2108] Loading core specs affects startup time Created: 13/Feb/17  Updated: 13/Feb/17

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

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

Approval: Vetted

 Description   

Adding the loading of spec itself + the clojure.core.specs namespace containing specs for core makes start time worse (and will only get longer as we add more core specs).



 Comments   
Comment by Ghadi Shayban [ 13/Feb/17 4:58 PM ]

Locally,
alpha14 takes 1.02s

time java -jar clojure-1.9.0-alpha14.jar -e ':foo'

Master with this line commented out takes 0.92s

time java -jar target/clojure-1.9.0-master-SNAPSHOT.jar -e ':foo'




[CLJ-2107] s/explain of a predicate defined s/with-gen yields s/unknown Created: 07/Feb/17  Updated: 08/Feb/17  Resolved: 08/Feb/17

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

Type: Defect Priority: Major
Reporter: Tamas Herman Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: generator, spec


 Description   

This explanation is clear:

(s/def ::some-string string?)
(s/explain ::some-string 123)

val: 123 fails spec: :boot.user/some-string predicate: string?

However if the failing spec has a custom generator, the explanation is not so helpful:

(s/def ::some-string-with-custom-generator (s/with-gen string? gen/string-alphanumeric))
(s/explain ::some-string-with-custom-generator nil)

=> :boot.user/some-string-with-custom-generator
val: nil fails spec: :boot.user/some-string-with-custom-generator predicate: :clojure.spec/unknown

I would expect the same predicate: string? explanation.

Based on the symptom I suspect this issue is related to http://dev.clojure.org/jira/browse/CLJ-2068



 Comments   
Comment by Tamas Herman [ 08/Feb/17 2:21 AM ]

Explanation of a workaround from Slack from @hiredman:

with-gen is a function, so it's arguments are evaluated, so the string? argument to with-gen is a function object, when spec is trying to find the name to report it does some stuff, which for symbols and keywords reports a good name, but for other Objects (including function objects) you get :clojure.spec/unknown.

wrapping with s/spec allows the spec macro to capture the meaningful, the symbol before evaluation:

(s/def ::some-string-with-custom-generator (s/with-gen (s/spec string?) gen/string-alphanumeric))
(s/explain ::some-string-with-custom-generator nil)

val: nil fails spec: :boot.user/some-string-with-custom-generator predicate: string?

I don't see any simple solution to make the original example yield a good explanation,
so maybe we should just explain how it works in https://clojure.org/guides/spec#_custom_generators
and wrap the with-gen examples with spec.

Comment by Alex Miller [ 08/Feb/17 8:49 AM ]

As you mentioned, this is a dup of CLJ-2068 and is fixed by the patch there.





[CLJ-2106] satisfies? is quite slow when returning false Created: 06/Feb/17  Updated: 09/Feb/17  Resolved: 09/Feb/17

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

Type: Enhancement Priority: Minor
Reporter: Mike Kaplinskiy Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: performance


 Description   
(defprotocol SatisfiesTest (testThing [this]))
(defrecord MyRecord [] Object SatisfiesTest (testThing [this]))
(def r (MyRecord.))

(time (dotimes [_ 30000] (instance? user.SatisfiesTest r)))
"Elapsed time: 1.715 msecs"

(time (dotimes [_ 30000] (satisfies? SatisfiesTest r)))
"Elapsed time: 3.944 msecs"

(time (dotimes [_ 30000] (instance? user.SatisfiesTest {})))
"Elapsed time: 2.304 msecs"

(time (dotimes [_ 30000] (satisfies? SatisfiesTest {})))
"Elapsed time: 718.949 msecs"

It would be nice if satisfies? memoized negative return values by class (though that cache would need to be cleared by `extend-type` and friends).



 Comments   
Comment by Alex Miller [ 06/Feb/17 1:56 PM ]

This is covered in an existing ticket - http://dev.clojure.org/jira/browse/CLJ-1814

Comment by Alex Miller [ 06/Feb/17 2:02 PM ]

Actually, read too quickly - CLJ-1814 covers the positive case only. I don't think we're going to cache a negative return value though. Managing and cleaning that cache is likely to not be worth it. If you need this kind of thing, you should probably consider a different kind of conditional check before-hand.

Comment by Alex Miller [ 06/Feb/17 2:05 PM ]

For example, you can just memoize this call like this:

user=> (def check (memoize #(satisfies? SatisfiesTest %)))
#'user/check
user=> (time (dotimes [_ 30000] (check {})))
"Elapsed time: 3.512 msecs"
Comment by Alex Miller [ 06/Feb/17 2:13 PM ]

I tweaked the test to remove the use of range and the construction of the record so the numbers are more useful.

Comment by Nicola Mometto [ 08/Feb/17 6:52 PM ]

Alex Miller CLJ-1814 handles the negative cases too, and doesn't keep a cache of negative return values. It keeps a cache of all the protocols extended by a certain class and invalidates that cache on every call to extend, the benchmarks on the ticket description showcase both a positive and a negative test

Comment by Alex Miller [ 09/Feb/17 3:09 PM ]

Nicola - cool! I didn't realize that. Will mark this as a dupe then.





[CLJ-2105] incorrect spec conform when an optional (?) is inside of a one or more (+) Created: 03/Feb/17  Updated: 03/Feb/17

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

Type: Defect Priority: Major
Reporter: Anthony D'Ambrosio Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

1.9.0-alpha14


Approval: Vetted

 Description   
(s/def ::thing (s/cat :a (s/? string?) :b (s/+ number?))) 
(s/def ::seq-of (s/+ ::thing))

(s/conform ::seq-of (list "foo" 1 "bar" 2 3 "qux" 4)) 
;=> [{:a "foo", :b [1]} [{:a "bar", :b [2 3]} {:a "qux", :b [4]}]]

;EXPECTED 
;=> [{:a "foo", :b [1]} {:a "bar", :b [2 3]} {:a "qux", :b [4]}]

;; ONLY 2nd thing matters? 
(s/conform ::seq-of (list "foo" 1 2 "bar" 3)) 
;=> [{:a "foo", :b [1 2]} {:a "bar", :b [2]}]

;; NO OPTIONAL 
(s/def ::thing (s/cat :a string? :b (s/+ number?))) 
(s/def ::seq-of (s/+ ::thing))
(s/conform ::seq-of (list "foo" 1 "bar" 2 3 "qux" 4)) 
;=> [{:a "foo", :b [1]} {:a "bar", :b [2 3]} {:a "qux", :b [4]}]

This also only shows up if there are 2+ numbers in the 2nd or later ::thing
AND the problem goes away if I make :a not optional...

Could be related to http://dev.clojure.org/jira/browse/CLJ-2003 ?






[CLJ-2104] Typo in pprint docstring Created: 29/Jan/17  Updated: 29/May/17

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, typo

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

 Description   

Typo in pprint ns docstring: "complete documentation on the the clojure web site on github"



 Comments   
Comment by Cameron Desautels [ 29/May/17 12:00 PM ]

While we're at it, might as well fix the orthography of "clojure" (=> "Clojure") and "github" (=> "GitHub").





[CLJ-2103] s/coll-of and s/every gen is very slow if :kind specified without :into Created: 28/Jan/17  Updated: 29/Jun/17

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

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

alpha14


Attachments: Text File clj-2103.patch    
Patch: Code
Approval: Screened

 Description   

An s/coll-of with :kind but without :into takes a very long time to generate. You will mostly notice this when using stest/check but here is an example that replicates the important bits:

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

;; Basic coll-of - fast
(time (dorun (gen/sample (s/gen (s/coll-of int?)) 1000)))
;; "Elapsed time: 49.10318 msecs"

;; coll-of with :into - fast
(time (dorun (gen/sample (s/gen (s/coll-of int? :into ())) 1000)))
;; "Elapsed time: 50.169762 msecs"

;; coll-of with :kind - SLOW
(time (dorun (gen/sample (s/gen (s/coll-of int? :kind list?)) 1000)))
;; "Elapsed time: 14922.123514 msecs"

;; coll-of with :kind and :into - fast
(time (dorun (gen/sample (s/gen (s/coll-of int? :kind list? :into ())) 1000)))
"Elapsed time: 54.188577 msecs"

Cause: If :into is not provided, s/every and s/coll-of has to generate a collection (via gen of list?) to call empty on it, to then fill it up. This is quite clearly documented in the docstring of `s/every`:

:kind - a pred/spec that the collection type must satisfy, e.g. vector?
         (default nil) Note that if :kind is specified and :into is
         not, this pred must generate in order for every to generate.

Assumedly the list? generates quite large vectors at a certain point which significantly slows down the generation. The responsible code is in gen* of every-impl.

Proposed: Use standard empty coll for persistent collection :kind preds (list? vector? set? map?).

In the patch:

  • Create lookup table `empty-coll` for kind-form (resolved pred) to empty coll.
  • When possible (if :into coll is specified or kind-form is one of these special cases), determine the empty gen-into collection once at the beginning.
  • If empty gen-into collection is available, start with that in the generator. Otherwise, fall back to prior behavior for kind.

Performance-wise, the times after the patch are comparable to times using :into.

Patch: clj-2103.patch



 Comments   
Comment by Leon Grapenthin [ 14/Feb/17 3:03 PM ]

I see two approaches to improve this behavior:

1. The gen uses the gen of kind to generate one value with the smallest size, calls empty on it to determine :into. This would lead to a surprise when your :kind is e. g. (s/or :a-vec vector? :a-list list?) (which currently throws, anyway)
2. We use an internal lookup table to assume :into. {clojure.core/vector? [], clojure.core/set? #{} ...}





[CLJ-2102] Reduce collection generator default size from 20 Created: 25/Jan/17  Updated: 12/May/17

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: generator, spec

Attachments: Text File clj-2102-2.patch     Text File clj-2102-3.patch     Text File clj-2102.patch    
Patch: Code
Approval: Incomplete

 Description   

In general I find that it is very easy (especially with nested or recursive collections) to have a check run OOME due to generating very large nested collections. Currently the default is 20 - I think we should change it to 3.

The attached patch just changes the default from 20 to 3. An alternate approach would be to change it to a dynvar setting.

Patch: clj-2102-3.patch



 Comments   
Comment by Alex Miller [ 07/Apr/17 3:45 PM ]

Updated patch to apply to master

Comment by Alex Miller [ 10/May/17 12:54 PM ]

Updated patch to apply to spec.alpha

Comment by Stuart Halloway [ 12/May/17 10:32 AM ]

I have definitely seen the pain here – nested collections can get big fast. OTOH for non-nested collections the larger generator is nice. Not sure moving the default is a help.

Comment by Alex Miller [ 12/May/17 3:23 PM ]

Use dynamic var and reduce default. Also consider ways to avoid this kind of problem in test.check itself (how does quickcheck deal with this?).





[CLJ-2101] Request for a way to forget specs Created: 24/Jan/17  Updated: 25/Jan/17  Resolved: 24/Jan/17

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

Type: Enhancement Priority: Trivial
Reporter: Johan Gall Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: spec


 Description   

Hello,

I have a server that loads code, and specs, at runtime in generated namespaces.

Thus I have a spec leak.

It would be nice if there was an official API to forget specs.



 Comments   
Comment by Johan Gall [ 24/Jan/17 7:01 AM ]

(or better, a way to create a temporary spec world, but then that is probably not trivial)

Comment by Alex Miller [ 24/Jan/17 7:51 AM ]

Is this covered by http://dev.clojure.org/jira/browse/CLJ-2060 ?

Comment by Alex Miller [ 24/Jan/17 12:51 PM ]

Marking as a dup for now, but will re-open if this goes beyond what's in CLJ-2060.

Comment by Johan Gall [ 25/Jan/17 5:20 AM ]

ah sorry, indeed it's a dup





[CLJ-2100] s/nilable form should include the original spec, not the resolved spec Created: 19/Jan/17  Updated: 14/Mar/17  Resolved: 14/Mar/17

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

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

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

 Description   
=> (clojure.spec/def :test/name string?)
:test/name

=> (clojure.spec/form (clojure.spec/and :test/name))
(clojure.spec/and :test/name)

=> (clojure.spec/form (clojure.spec/nilable :test/name))
(clojure.spec/nilable clojure.core/string?)

In the final line, s/nilable form has the resolved spec rather than the original spec.

Proposed: Instead of getting the internal spec description, resolve the original spec form.

After:

user=> (clojure.spec/form (clojure.spec/nilable :test/name))
(clojure.spec/nilable :test/name)

Patch: CLJ-2100.patch






[CLJ-2099] Keywords with aliased namespaces cannot be read when the namespace is required in a reader conditional Created: 13/Jan/17  Updated: 25/Jan/17  Resolved: 13/Jan/17

Status: Closed
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: Declined Votes: 0
Labels: keywords, reader, readerconditionals


 Description   

The title in itself isn't entirely true, but I couldn't find a way to describe it succintly (feel free to change).

The issue is easier to demonstrate with an example:

(ns foo
  #?(:cljs (:require [clojure.core :as c])))

#?(:cljs ::c/x)

When reading this in a :clj context, the reader cannot read ::c/x ("Invalid token: ::c/x"), despite the code being correct (presumably).
The same thing happens if the reader conditional branches are :clj and the source is read in a :cljs context.



 Comments   
Comment by Alex Miller [ 13/Jan/17 8:07 AM ]

This looks like expected behavior to me. Auto-resolved keywords rely on a resolution context and there isn't one at the point where ::c/x is being read.

Comment by Viktor Magyari [ 13/Jan/17 9:05 AM ]

To me it seems reasonable to expect the resolution context to include the clojure.core alias - more generally, include <platform> specific aliases in the <platform> branches of reader conditionals. Maybe consider this as an enhancement ticket?

Comment by Alex Miller [ 13/Jan/17 9:18 AM ]

To do this would require adding special handling specifically for ns or other things that create aliases, which implies conditional evaluation of some forms at read-time. You would also need some place (other than the current platform's namespace maps) to track other platform's namespace aliases. That's a lot of very special, custom stuff.

We're not going to add this.

Comment by Leon Grapenthin [ 25/Jan/17 2:13 PM ]

1. Why does the reader try to read the :cljs branch in Viktors example?
2. The original intent of reader conditionals was that the code could be read independently of the lang. Have aliases not been considered then due to a lack of popularity of ::a/n syntax?

My suggestion would be a knob that reads unresolvable alias kws as a special object #aliased-keyword{:alias "a", :name "n"}. This would solve both Viktors problem and also pay its debt to the reader conditional intent.

Comment by Alex Miller [ 25/Jan/17 5:16 PM ]

1. The reader reads everything, it's just selective about which parts of an expression gets returned. Without reading, it wouldn't know where the end of that form was.

2. I think this is a little more subtle in that reading the second thing in a conditional requires remembering something read and discarded in a prior conditional.

That said, maybe it would be possible to either read in a mode where this particular case doesn't throw or where this particular exception was known and discarded when inside a conditional.





[CLJ-2098] autodoc fails to load clojure/spec.clj Created: 12/Jan/17  Updated: 07/Apr/17

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

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

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

 Description   

Reported by Tom F for the autodoc process. The following (essentially) is what autodoc is doing that currently blows up:

tom@renoir:~/src/clj/autodoc-work-area/clojure/src$ java -cp clojure.jar clojure.main
Clojure 1.9.0-master-SNAPSHOT
user=> (load-file "src/clj/clojure/spec.clj")
CompilerException java.lang.IllegalArgumentException: No implementation of method: :conform* \
of protocol: #'clojure.spec/Spec found for class: clojure.spec$regex_spec_impl$reify__14279, \
compiling:(/home/tom/src/clj/autodoc-work-area/clojure/src/src/clj/clojure/spec.clj:684:1)

Cause: There is code in Compiler.macroexpand() with the intention to suspend spec checking inside clojure.spec. However, it is currently doing an exact path match on SOURCE_PATH. In the load-file call above, this ends up being an absolute path.

Approach: Check for a path suffix rather than an exact match in Compiler.

Patch: clj-2098.patch



 Comments   
Comment by Alex Miller [ 07/Apr/17 3:38 PM ]

Removing "code" tag for patch as this is not good to screen yet. While the patch kind of works, it would be good if there were a more reliable way to omit spec checking in the scope of the spec namespace. Rather than checking the source path, it would be much better to instead check the current ns. This works EXCEPT during the macro expansion of ns (which has a spec) in spec itself. At that point you have not yet evaluated the (in-ns 'clojure.spec) (since that's what the ns macro is expanding to). Really, all of this is a problem at exactly one point - compiling clojure.spec itself. Another option might be some way to suspend macro spec checking altogether (which might be a generally useful feature).





[CLJ-2097] Improve generation failure exception Created: 10/Jan/17  Updated: 11/Jan/17

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

Type: Enhancement Priority: Major
Reporter: Russell Mull Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: generator, spec


 Description   

It's pretty easy to write a spec whose generator fails like this:

Couldn't satisfy such-that predicate after 100 tries.

This is of course expected in many ways, but it's a very unhelpful error. Some things that could make this better include:

  • Including the spec that failed in the exception. I only see one invocation of gen/such-that in spec.clj, and it appears to have the spec's form at hand. gen/such-that takes an exception constructor where this could be used.
  • Allow max-tries to be changed from the hardcoded value of 100. When dealing with an intermittent failure, it can be useful to crank down max-tries to a very small number, making the failure easier to reproduce.


 Comments   
Comment by Alex Miller [ 11/Jan/17 8:41 AM ]

These are reasonable suggestions and this area is likely to evolve in tandem with test.check to provide better info.





[CLJ-2096] "Key must be integer" thrown when vector lookup done with short or byte Created: 04/Jan/17  Updated: 02/Jun/17  Resolved: 05/Jan/17

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

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

clojure-1.9.0-alpha14 and clojure-1.8.0 tested.



 Description   

Looking up a value in a vector with a long or integer index works:
([:a :b :c] (long 1)) => :b
([:a :b :c] (int 1)) => :b

However, lookups with short or byte fail with "IllegalArgumentException Key must be integer"
([:a :b :c] (short 1))
([:a :b :c] (byte 1))

Root cause seems to be clojure.lang.Util.isInteger() which returns true only if the argument is an instance of Integer, Long, clojure.lang.BigInt, or BigInteger. I think it would be more correct for clojure.lang.Util.isInteger() to be consistent with clojure.core/integer? which additionally returns true for both Short and Byte.



 Comments   
Comment by Alex Miller [ 05/Jan/17 8:24 AM ]

I don't see any good reason to make changes in this area so declining.

Comment by Aaron Cummings [ 05/Jan/17 8:44 AM ]

Hi Alex - the case where we ran into this exception was in parsing a binary file where the record descriptor is a byte, and we used this byte to do a lookup in a vector (the vector holding keywords which describe the record type). The workaround is pretty simple though; just cast the byte to an int.

Curiously, a map lookup like ({0 :a, 1 :b, 2 :c} (byte 1)) does work.

I wondering though if the non-support of short and byte lookups in vectors is intentional, and if so, the reason for this choice (I don't see any obvious problems, so perhaps Rich knows something I don't here). If instead this is an oversight, and is deemed not broken enough to fix, then I can accept that.

Comment by Alex Miller [ 06/Jan/17 8:26 AM ]

I would say that given that the check and error message exists, it is intentional. Certainly there is a performance impact to checking for a broader set of types.

Comment by Daniel Balke [ 02/Jun/17 9:45 AM ]

I ran into this while processing byte arrays and expected it to work because (= (byte 1) 1).
To address your points, Alex:

I would argue it's unintentional because the error message only indicates that the argument is not an integer, which bytes and shorts by definition of clojure.core/integer? and clojure.core/int? clearly are.
nth does work while using the vector as a function does not.

As for the performance impact: It would only slow down if you use bytes or shorts as arguments, else it would short-circuit, so old code would be unchanged in raw performance.

If you still think this is too unimportant to fix, that's also fine, just wanted to hear your opinions on this.





[CLJ-2095] Doc s/gen overrides do not take effect inside custom generators Created: 03/Jan/17  Updated: 03/Jan/17

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

Type: Enhancement Priority: Minor
Reporter: Russell Mull Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, generator, spec
Environment:

clojure 1.9.0-alpha14



 Description   

Custom generators may build (via fmap/bind) on spec generators. Generator overrides at the top level will not take effect inside custom generators:

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

;; A map that holds a single integer value
(s/def ::val integer?)
(s/def ::body (s/keys :req [::val]))

;; This spec matches stringified versions of 'body'.
;; (read-string is for demonstration purposes only, of course)
(s/def ::stringy-body
  (s/with-gen
    (s/and string? #(s/valid? ::body (read-string %)))
    #(gen/fmap pr-str (s/gen ::body))))

(s/valid? ::stringy-body "{:user/val 37}") ;; => true

;; This makes various stringified maps, as expected
(take 3 (gen/sample (s/gen ::stringy-body)))
;; => ("#:user{:val -1}" "#:user{:val 0}" "#:user{:val -1}")

;; *** But the overrides don't get passed through ***
(take 3 (gen/sample (s/gen ::stringy-body {::val #(s/gen #{42})})))
;; ("#:user{:val -1}" "#:user{:val 0}" "#:user{:val 0}")

Should consider documenting this in s/gen, s/with-gen, etc.



 Comments   
Comment by Alex Miller [ 03/Jan/17 5:39 PM ]

When you use with-gen, you're basically overriding the built-in gen mechanism (which supports overrides) and providing your own (opaque to spec) generator. You should not expect overrides to take effect inside a custom generator.

Comment by Russell Mull [ 03/Jan/17 5:41 PM ]

That makes sense, but in lieu of that I expected (and went looking for) some way to get at the overrides map from the function passed to s/with-gen, and found none.

Comment by Russell Mull [ 03/Jan/17 5:48 PM ]

... I didn't fully parse your comment the first time around. I can see from the implementation that a custom generator (gfn internally) is never passed any of the contextual information that the builtin specs have at hand. As it sounds like this is intentional, it would be useful to note this limitation in the docstring for s/gen or perhaps s/with-gen.

Comment by Alex Miller [ 03/Jan/17 5:58 PM ]

It's not a crazy idea, but it doesn't seem like there's any way this could be done in the current impl without some pretty significant changes.





[CLJ-2094] clojure.spec: bug with protocols and extend-type Created: 01/Jan/17  Updated: 03/Jan/17  Resolved: 03/Jan/17

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

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


 Description   

I have the following two clj files (I've tried to come up with a minimal example):

core.clj
--------------
(ns spec-test.core
  (:require [clojure.spec :as s]))

(defprotocol Game
  (move [game]))

(s/def ::game1 #(satisfies? Game %))
(s/def ::game2 (partial satisfies? Game))

foo.clj
--------------
(ns spec-test.foo
  (:require [spec-test.core :refer [Game]]))

(defrecord Foo [])

(extend-type Foo
  Game
  (move [game]))

Here's a REPL session that explains my problem:

user=> (ns spec-test.core)
nil
spec-test.core=> (require 'spec-test.core :reload)
nil
spec-test.core=> (require 'spec-test.foo :reload)
nil
spec-test.core=> (satisfies? Game (spec-test.foo/->Foo))
true
spec-test.core=> ((partial satisfies? Game) (spec-test.foo/->Foo))
true
spec-test.core=> (s/explain ::game1 (spec-test.foo/->Foo))
Success!
nil
spec-test.core=> (s/explain ::game2 (spec-test.foo/->Foo))
val: #spec_test.foo.Foo{} fails spec: :spec-test.core/game2 predicate: (partial satisfies? Game) <---- WAAAAAT
nil

I would expect ::game1 and ::game2 to be equivalent, but somehow they're not.

Also see: https://groups.google.com/forum/#!topic/clojure/igBlMpqGU3A



 Comments   
Comment by Steve Miner [ 02/Jan/17 12:43 PM ]

I gave some incorrect advice on the mailing list so I'll try to correct myself here. Basically, the protocol is stored in a var, Game. If a predicate captures a particular state of the protocol, it won't respond correctly when additions are made to the protocol (with extend-type, etc.) So the problem with the usage of `partial` is that it evaluates the current value of the protocol at that point in time, before it has been extended to cover Foo.

The #(...) function definition would have used the var itself, not its current value. Naturally, the var allows the protocol to be updated such that the function sees the updated value. Basically, this is just normal Clojure evaluation. A `defn` style function would have worked fine as well. It's just the `partial` that evaluated its args that leads to the problem.

The same kind of issue could come up if you passed any var to partial and captured the current value of the var. Later changes to the var would not affect the result of partial.

I'll say the bug is the confusing error message. It seems that s/explain is implying it is using the var Game, but it really captured an "old" value of Game.

Comment by Alex Miller [ 03/Jan/17 2:10 PM ]

I think Steve's assessment is all correct.

I'm not sure that it's possible for spec to know what's happened here though wrt giving a better error message.

I don't think I really see anything that can be "fixed", so I'm going to mark this as declined.





[CLJ-2093] partial and fn behave differently with eval Created: 28/Dec/16  Updated: 31/Dec/16  Resolved: 31/Dec/16

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

Type: Defect Priority: Major
Reporter: N/A Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

(eval (list map (partial + 1) [0]))
CompilerException java.lang.ExceptionInInitializerError
(eval (list map (fn [x] (+ 1 x)) [0]))
=> (1)



 Comments   
Comment by Kevin Downey [ 29/Dec/16 9:22 PM ]

same issue as http://dev.clojure.org/jira/browse/CLJ-1206 and all the issues connected to that

Comment by Alex Miller [ 31/Dec/16 11:17 AM ]

You should not expect either of these to work as the expressions contain function objects (not function forms).

You should be doing something like this:

(eval (list 'map '(partial + 1) [0]))




[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-2091] clojure.lang.APersistentVector#hashCode is not thread-safe Created: 24/Dec/16  Updated: 12/May/17  Resolved: 12/May/17

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

Type: Defect Priority: Major
Reporter: thurston n Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: collections, concurrency

Attachments: File clj-2091-0.diff     File clj-2091-default-initialization.diff    
Patch: Code
Approval: Ok

 Description   

Problem: clojure.lang.APersistentVector#hashCode contain a deliberate data race on hash computation. However, the code as written does not follow safe practices for the intended data race. Specifically, the problem arises because the hashCode() (and hasheq()) method make multiple reads of the (unsynchronized) _hash field. The JMM permits these reads to return different values. Specifically, the last read in the return may return the pre-computed value -1, which is not the desired hash value. This problem also applies to APersistentMap, APersistentSet, and PersistentQueue.

See: http://jeremymanson.blogspot.com/2008/12/benign-data-races-in-java.html for a good description of the problem.

Fix: The main fix is to read the cached hash field only once and return the value of the local computation, not the value of the field.

A secondary change that is also beneficial is to use the default initializer value (which has special ordering in the JMM to the beginning of the thread) rather than setting and using -1 as the sentinel value.

In both cases these changes follow the canonical idioms used in java.lang.String for lazy hash computation. The patch covers both.

Patch: clj-2091-default-initialization.diff - note that this patch will indicate whitespace errors when applied due to the wacky line endings in PersistentQueue. The problem here is really the PQ formatting, not the patch.

Prescreened by: Alex Miller

There are some hash-related tests already but I also spot-checked that hash computations are returning the same value with and without the patch for the collections in question.



 Comments   
Comment by thurston n [ 24/Dec/16 4:38 PM ]

I can of course provide a patch, but as this is my first issue and am generally unfamiliar with clojure's practices and because this issue is not restricted to APersistentVector#hashCode, I thought it best to hold off and let the stewards decide how to proceed and how I could best help

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

Patch welcome (but please sign the contributor's agreement first - http://clojure.org/community/contributing). Also, see the processes for developing patches at http://dev.clojure.org/display/community/Developing+Patches.

AFAICT, the affected classes are APersistentVector, APersistentMap, APersistentSet, and PersistentQueue? Would be ok by me to cover all of them in a single patch.

Comment by thurston n [ 03/Jan/17 4:00 PM ]

AFAICT, the affected classes are APersistentVector, APersistentMap, APersistentSet, and PersistentQueue?

  • Dunno. However my experience tells me that the broken idiom (racy cache/memoization) is likely elsewhere; but I know of no systematic way of finding them. Regardless, I'll just focus on those 4 classes.
  • My plan is to also fix #hasheq(). It's the same problem; if you don't want that then just let me know and I'll refrain.
  • I'm not planning to deal with the initialization of #_hash and #_hasheq (currently inline-initialized to -1); that's a separate (although related) thread-safety problem. Might they be just what we refer to as a "legacy idiosyncrasy"? If so, then they really should be changed to just be default-initialized. I did, as an experiment, change one to default initialization, and the tests passed - that should be enough, but given that the persistent classes are serializable, code-coverage, et al., I can't say for sure. So I'll leave it to others more familiar with the codebase to make that determination. I note that if it is in future determined to change them, then #hashCode() and #hasheq() will need to be modified (trivially) accordingly.

That's the plan. Sound good?

Comment by Alex Miller [ 03/Jan/17 9:49 PM ]

I thought the non-default initialization was part of what you were describing, so now I'm not sure we're on the same page. Maybe you can just patch one so we have something concrete to talk about.

Comment by thurston n [ 03/Jan/17 9:56 PM ]

I'm not sure what you mean by "patch one" - I just submitted a patch, did you look at that?

Comment by Alex Miller [ 03/Jan/17 10:13 PM ]

I meant one class - sorry, I didn't see the patch. I will look at it tomorrow with fresh eyes.

Comment by thurston n [ 03/Jan/17 11:02 PM ]

Sure.

To be clear, as I mentioned in today's earlier comment, I would advise removing the inline-initialization, viz.

int _hash = -1;
int _hasheq = -1;

with

int _hash;
int _hasheq;

As I wrote, the extant tests would pass (of course, changing #hashCode() and #hasheq() appropriately)

But the initialization issue is a different, although certainly not orthogonal, issue than the one my patch addresses.

Currently, (i.e. pre-patch), #hashCode() can return a spurious -1 even if an APersistentVector instance is safely published - my patch fixes that.

However, because of the inline-initialization, an APersistentVector instance that is not safely published could return a spurious 0 from #hashCode(), even with my patch.

Now if the inline-initialization is just a "legacy idiosyncrasy" (and we all do that at one time or another), then it could be safely replaced (along with the appropriate modification to my patch) and all APersistentVector instances (safely published or not), would have #hashCode() implementations that are correct.

Comment by Alex Miller [ 06/Jan/17 3:19 PM ]

Ok, I went over all this again and it makes sense to me. I think you should proceed and also make the initializer change (remove the -1 as sentinel and replace with no initializer and 0 for the comparison checks in the methods).

Comment by thurston n [ 06/Jan/17 11:36 PM ]

Combines the 2 commits into a single commit patch
So incorporates the original patch changes (single read) with default initialization and checks for zero
Don't know what to do with PersistentQueue's mixed line-endings – that I'll leave to you to deal with

Comment by thurston n [ 09/Jan/17 8:16 PM ]

Problem also in core.lang.ASeq#hashCode() and core.lang.ASeq#hasheq() - although thankfully without inline initialization

Surely not the last place either

Comment by Alex Miller [ 10/Jan/17 8:33 AM ]

Feel free to update the patch if you like





[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-2089] Sorted colls with default comparator don't check that first element is Comparable Created: 19/Dec/16  Updated: 12/May/17

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

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

Attachments: Text File clj-2089.patch    
Patch: Code and Test
Approval: Screened

 Description   

Sorted maps and sets use the default comparator. The default comparator requires elements to be Comparable. PersistentTreeMap will not actually invoke the comparator until the second element is added to the map/set, so it is possible to create an invalid single element sorted map/set that will blow up only on later use.

The following examples create invalid "timebomb" collections that will throw ClassCastException if another element is added or if they're used in other ways (because sets are not comparable):

(sorted-set #{1})
(conj (sorted-set) #{1})
(sorted-map #{} 1)
(assoc (sorted-map) #{} 1)

Example:

(def s (sorted-set #{1}))  ;; this doesn't fail
(conj s #{2})              ;; first conj triggers error
ClassCastException clojure.lang.PersistentHashSet cannot be cast to java.lang.Comparable  clojure.lang.Util.compare (Util.java:153)

Cause: In PersistentTreeMap.add(), in the case where the existing tree is null, the comparator is never invoked and so the default comparator will never throw the ClassCastException seen on later compares. Note that none of this applies for a custom comparator (sorted-set-by and sorted-map-by) - those comparators can do whatever.

Proposed: In add(), if the map is empty AND the comparator is the default comparator, then check whether the added key is Comparable and throw CCE if not. Note that PersistentTreeMap is also the impl used by PersistentTreeSet so this covers both. The error message is customized to give you a better hint about the problem (and written to be applicable for both maps and sets). In the patch some existing (bad) tests had to be adjusted.

user=> (def s (sorted-set #{1}))
ClassCastException Default comparator requires nil, Number, or Comparable: #{1}  clojure.lang.PersistentTreeMap.add (PersistentTreeMap.java:335)

One downside of the current patch is that does not catch the equivalent problem if using a custom comparator and a bad first element. You could maybe do this by calling comp.compare(k,k) (although many comparators, like the default comparator, have an early identity check that would not fail on this). For now, decided that if you supply a custom comparator, it's up to you to not use it with elements that don't work with that comparator.

Patch: clj-2089.patch

Screener Notes: The error is a ClassCastException in order to match the exception that would have come later, but is odd in that no class casting was done. Maybe IllegalArgumentException?






[CLJ-2088] 'into' can make maps from collection of lists, but vectors are ok. Created: 19/Dec/16  Updated: 20/Dec/16