<< Back to previous view

[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-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-1218] mapcat is too eager Created: 16/Jun/13  Updated: 22/Apr/17

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

Type: Enhancement Priority: Minor
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: lazy

Attachments: Text File CLJ-1218-lazier-mapcat.patch    
Patch: Code and Test

 Description   

The following expression prints 1234 and returns 1:

(first (mapcat #(do (print %) [%]) '(1 2 3 4 5 6 7)))

The reason is that (apply concat args) is not maximally lazy in its arguments, and indeed will realize the first four before returning the first item. This in turn is essentially unavoidable for a variadic concat.

This could either be fixed just in mapcat, or by adding a new function (to clojure.core?) that is a non-variadic equivalent to concat, and reimplementing mapcat with it:

(defn join
  "Lazily concatenates a sequence-of-sequences into a flat sequence."
  [s]
  (lazy-seq (when-let [[x & xs] (seq s)] (concat x (join xs)))))


 Comments   
Comment by Gary Fredericks [ 17/Jun/13 7:54 AM ]

I realized that concat could actually be made lazier without changing its semantics, if it had a single [& args] clause that was then implemented similarly to join above.

Comment by John Jacobsen [ 27/Jul/13 8:08 AM ]

I lost several hours understanding this issue last month [1, 2] before seeing this ticket in Jira today... +1.

[1] http://eigenhombre.com/2013/07/13/updating-the-genome-decoder-resulting-consequences/

[2] http://clojurian.blogspot.com/2012/11/beware-of-mapcat.html

Comment by Gary Fredericks [ 05/Feb/14 1:35 PM ]

Updated join code to be actually valid.

Comment by Ghadi Shayban [ 21/May/15 8:32 PM ]

The version of join in the description is not maximally lazy either, and will realize two of the underlying collections. Reason: destructuring the seq results in a call to 'nth' for 'x' and 'nthnext' for 'xs'. nthnext is not maximally lazy.

(defn join
  "Lazily concatenates a sequence-of-sequences into a flat sequence."
  [s]
  (lazy-seq
   (when-let [s (seq s)] 
     (concat (first s) (join (rest s))))))




[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-440] java method calls cannot omit varargs Created: 27/Sep/10  Updated: 21/Apr/17

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

Type: Enhancement Priority: Critical
Reporter: Alexander Taggart Assignee: Ragnar Dahlen
Resolution: Unresolved Votes: 13
Labels: interop

Attachments: Text File 0001-CLJ-440-Allow-calling-vararg-Java-methods-without-va.patch    
Approval: Triaged

 Description   

Problem

Clojure calls to Java vararg methods require creating an object array for the final arg. This is a frequent source of confusion when doing interop.

E.g., trying to call java.util.Collections.addAll(Collection c, T... elements):

user=> (Collections/addAll [] (object-array 0))
false
user=> (Collections/addAll [])
IllegalArgumentException No matching method: addAll  clojure.lang.Compiler$StaticMethodExpr.<init> (Compiler.java:1401)

The Method class provides an isVarArg() method, which could be used to inform the compiler to process things differently.

From http://groups.google.com/group/clojure/browse_thread/thread/7d0d6cb32656a621

Latest patch: Removed because incomplete and goal not clear

Varargs in Java

As currently stated, the scope of this ticket is only to omit varargs, but this is only one case where Clojures handling of varargs differs from Java. For completeness, here is a brief survey of how Java handles vararg methods, which could hopefully inform a discussion for how Clojure could do things differently, and what the goal of this ticket should be.

Given the following setup:

VarArgs.java
public class VarArgs {

    public static class SingleVarargMethod {
        public static void m(String arg1, String... args) {}
    }

    public static class MultipleVarargMethods {
        public static void m(String... args) {}
        public static void m(String arg1) {}
        public static void m(String arg1, String... args) {}
    }
}
Java Possible clojure equivalent? Comments
VarArgs.SingleVarargMethod.m("a"); (SingleVarargMethod/m "a")  
VarArgs.SingleVarargMethod.m("a", "b"); (SingleVarargMethod/m "a" "b")  
VarArgs.SingleVarargMethod.m("a", "b", "c"); (SingleVarargMethod/m "a" "b" "c")  
VarArgs.SingleVarargMethod.m("a", new String[]{"b", "c"}); (SingleVarargMethod/m "a" (object-array ["b" "c"]))  
VarArgs.MultipleVarargMethods.m(); (MultipleVarargMethods/m)  
VarArgs.MultipleVarargMethods.m((String) null); (MultipleVarargMethods/m nil) Use type hints to disambiguate?
VarArgs.MultipleVarargMethods.m((String[]) null); (MultipleVarargMethods/m nil) Use type hints to disambiguate?
VarArgs.MultipleVarargMethods.m("a", null); (MultipleVarargMethods/m "a" nil)  
VarArgs.MultipleVarargMethods.m("a", new String[]{}); (MultipleVarargMethods/m "a" (object-array 0))  
VarArgs.MultipleVarargMethods.m(new String[]{"a"}); (MultipleVarargMethods/m (object-array ["a"]))  
VarArgs.MultipleVarargMethods.m("a", new String[]{"b", "c"}); (MultipleVarargMethods/m "a" (object-array ["b" "c"]))  


 Comments   
Comment by Assembla Importer [ 27/Sep/10 8:19 PM ]

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

Comment by Alexander Taggart [ 01/Apr/11 11:16 PM ]

Patch adds support for varargs. Builds on top of patch in CLJ-445.

Comment by Alexander Taggart [ 05/Apr/11 5:45 PM ]

Patch updated to current CLJ-445 patch.

Comment by Nick Klauer [ 29/Oct/12 8:12 AM ]

Is this ticket on hold? I find myself typing (.someCall arg1 arg2 (into-array SomeType nil)) alot just to get the right method to be called. This ticket sounds like it would address that extraneous into-array arg that I use alot.

Comment by Andy Fingerhut [ 29/Oct/12 10:45 AM ]

fixbug445.diff uploaded on Oct 29 2012 was written Oct 23 2010 by Alexander Taggart. I am simply copying it from the old Assembla ticket tracking system to here to make it more easily accessible. Not surprisingy, it doesn't apply cleanly to latest master. I don't know how much effort it would be to update it, but only a few hunks do not apply cleanly according to 'patch'. See the "Updating stale patches" section on the JIRA workflow page here: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Andy Fingerhut [ 29/Oct/12 10:56 AM ]

Ugh. Deleted the attachment because it was for CLJ-445, or at least it was named that way. CLJ-445 definitely has a long comment history, so if one or more of its patches address this issue, then you can read the discussion there to see the history.

I don't know of any "on hold" status for tickets, except for one or two where Rich Hickey has explicitly said in a comment that he wants to wait a while before making the change. There are just tickets that contributors choose to work on and ones that screeners choose to screen.

Comment by Alex Miller [ 02/Feb/16 11:47 AM ]

I would love to see an updated patch on this ticket that specifically addressed the varargs issue without building on the other mentioned ticket and patch (which is of lower priority).

Comment by Ragnar Dahlen [ 03/Feb/16 8:01 AM ]

I had a stab at this, have attached an initial patch, parts of which I'm not too sure/happy about so feedback would be appreciated.

The patch takes the following approach:

  1. Teach Reflector/getMethods how to find matching vararg methods. In addition to the current constraints, a method can also match if it is a varargs method, and the arity of the method is one more than the requested arity. That means it's a varargs method we could call, but the user hasn't provided the varargs argument.
  2. In MethodExpr/emitTypedArgs we handle the case were there is one more argument in the method being called than there were arguments provided. The only case were that should happen is when it is a varargs method and the last argument was not provided. In that case we push a new empty object array to the stack.

I'm not to sure about my implementation of the second part. It could open up for some hard to understand bugs in the future. One option would be to be more defensive, and make sure it's really the last argument for instance, or even pass along the Method object (or a varargs flag) so we know what we can expect and need to do.

Comment by Ragnar Dahlen [ 03/Feb/16 8:49 AM ]

I realised my patch is missing two important cases; the interface handling in Reflector and handling multiple matching methods. I'll look into that too, but would still appreciate feedback on the approach in MethodExpr/emitTypedArgs.

Comment by Alex Miller [ 03/Feb/16 9:00 AM ]

I am in favor of using isVarArg() to explicitly handle this case rather than guessing if we're in this situation. We should check the behavior (and add tests where it seems needed) for calling a var args method with too few args, too many args, etc. And also double-check that non vararg cases have not changed behavior.

Also, keep in mind that as a general rule, existing AOT compiled code may rely on calling into public Reflector methods, so if you change the signatures of public Reflector methods, you should leave a version with the old arity that has some default behavior for backwards compatibility.

Comment by Ghadi Shayban [ 21/Apr/17 11:31 AM ]

Any extra logic that the compiler implements will need to distinguish between:

public static class MultipleVarargMethods {
        public static void m(String... args) {}
        public static void m(String arg1, String... args) {}
    }

which I don't think is possible generically, without breaking code.

Rather than omitting varargs, how about handling them without tedious array construction. An alternative is to introduce new explicit sugar that you have to opt-in to:

(Whatever/varargs a b c ... x y z)

where ... or similar would be understood by StaticMethodExpr or InstanceMethodExpr in the compiler, and could be type-hinted to resolve ambiguity. This would not be a breaking change.





[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-2091] clojure.lang.APersistentVector#hashCode is not thread-safe Created: 24/Dec/16  Updated: 18/Apr/17

Status: Open
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: Unresolved 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-1906] Clojure should make representing iterated api calls easier Created: 30/Mar/16  Updated: 17/Apr/17

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

Type: Enhancement Priority: Minor
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: None

Attachments: Text File 0001-CLJ-1906-add-ingeminate-function.patch     Text File 0001-CLJ-1906-add-unfold-function.patch     Text File 0001-CLJ-1906-transducer-enabled-iterate.patch     File CLJ-1906-seqable-reducible.diff     Text File CLJ-1906-successions.patch    

 Description   

Many apis (elasticsearch, github, s3, etc) have parts of the api
which, in usage, end up being used in an interative way. You make an
api call, and you use the result to make another api call, and so
on. This most often shows up in apis have some concept of pages of
results that you page through, and is very prevalent in http apis.

This appears to be such a common pattern that it would be great if
Clojure had in built support for it.

You may think Clojure already does have support for it, after all,
Clojure has `iterate`. In fact the docstring for `iterate`
specifically says the function you give it must be free of side
effects.

I propose adding a function `unfold` to clojure.core to support this
use case. `unfold` would return an implementation of ReduceInit. The
name `unfold` matches what would be a similar Haskell function
(https://hackage.haskell.org/package/base-4.8.2.0/docs/Data-List.html#v:unfoldr)
and also matches the name for a similar function used in some existing
Clojure libraries
(https://github.com/amalloy/useful/blob/develop/src/flatland/useful/seq.clj#L128-L147).

`unfold` in some ways looks like a combination of `take-while` and
`iterate`, except for the fact that `iterate` requires a pure
function. Another possible solution would be a version of `iterate`
that doesn't require a pure function.

It seems like given the use case I envision for `unfold`, a
non-caching reducible would be perfect. But that would leave those
that prefer seqs high and dry, so maybe at least some consideration
should be given to seqs.

Mailing list discussion is here
(https://groups.google.com/forum/#!topic/clojure-dev/89RNvkLdYc4)

A sort of dummy api you might want to interact with would look something like

(import '(java.util UUID))

(def uuids (repeatedly 1000 #(UUID/randomUUID)))

(def uuid-index
  (loop [uuids uuids
         index  {}]
    (if (seq uuids)
      (recur (rest uuids) (assoc index (first uuids) (rest uuids)))
      index)))

(defn api
  "pages through uuids, 10 at a time. a list-from of :start starts the listing"
  [list-from]
  (let [page (take 10 (if (= :start list-from)
                        uuids
                        (get uuid-index list-from)))]
    {:page page
     :next (last page)}))

given the above api, if you had an implementation of `unfold` that took a predicate that decided when to continue unfolding, a producer which given a value in a sequence produced the next value, and an initial value, you could do something like this:

(= uuids (into [] (mapcat :page) (unfold :next (comp api :next) (api :start))))

and the result would be true.

The equivilant take-while + iterate would be something like:

;; the halting condition is not strictly the same
(= uuids (into [] (mapcat :page) (take-while (comp seq :page) (iterate (comp api :next) (api :start)))))


 Comments   
Comment by Kevin Downey [ 31/Mar/16 4:21 PM ]

I made two patches, one adds unfold as discussed above, one adds ingeminate which is like iterate but without the function purity restrictions, and doesn't return a seq.

Comment by Ghadi Shayban [ 11/Apr/16 10:46 AM ]

Though syntax is less important than the semantics, may I propose the name `progression` for this? Clojure's fold is called reduce, so unfold is too much like Haskell. Other names I was considering include evolve & derivations.

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

Another option would be `productions` (reminiscent of `reductions`).

Comment by Kevin Downey [ 11/Apr/16 9:32 PM ]

productions has a nice ring to it. emanate could work too, would sort near eduction

Comment by Ghadi Shayban [ 12/Apr/16 10:08 PM ]

Adding a patch with a generator impl that is clojure.lang.{Seqable,IReduceInit}.

Generative tests assert that the seq and reduce halves are equivalent.

Tests assert basic functionality, obeying reduced, and maximal laziness of the seq impl.

Docstring has been wordsmithed and the function named `productions`.

Comment by Kevin Downey [ 18/Apr/16 3:21 PM ]

apparently unfold is part of SRFI 1: List Library in scheme land http://srfi.schemers.org/srfi-1/srfi-1.html#FoldUnfoldMap

it looks like their unfold is take-while + iterate + map

Comment by Ghadi Shayban [ 23/Apr/16 11:06 PM ]

Main differences between Scheme's impl and this proposed one:
Predicate reversed (stop? vs continue?)
Scheme has a "mapping function" to produce a different value from the current seed, Clojure doesn't (but has transducers)
Scheme has an extra optional arg to build the tail of the list

Now I'm partial to the name successions.

Comment by Michał Marczyk [ 10/May/16 11:07 AM ]

I can confirm that I found unfold quite useful in my Scheme days.

In Clojure, this general pattern can be expressed using transducers at a modest cost in keystrokes:

(def numbers (doall (range 1000)))

(defn api [list-from]
  (if list-from
    (let [page (vec
                 (take 10 (if (= :start list-from)
                            numbers
                            (drop list-from numbers))))]
      {:page page
       :next (some-> (last page) inc)})))

(= numbers
   (sequence (comp (take-while some?)
                   (mapcat :page))
             (iterate (comp api :next)
                      (api :start))))
;= true

Maybe this could be simplified with an xform-enabled version of iterate?

(defn iterate*
  ([f seed]
   (iterate f seed))
  ([xform f seed]
   (sequence xform (iterate f seed))))

(= numbers
   (iterate*
     (comp (take-while some?) (mapcat :page))
     (comp api :next)
     (api :start)))
;= true

Admittedly this takes more characters, but is quite generic and a transducer-enabled overload in iterate feels pretty natural to me. Attaching a simple patch implementing this in clojure.core/iterate – I'll look at clojure.lang.Iterate to see if it's worth implementing direct support later, unless of course nobody wants this.

Comment by Michał Marczyk [ 10/May/16 11:08 AM ]

0001-CLJ-1906-transducer-enabled-iterate.patch adds a ternary overload to iterate that delegates to the binary overload and sequence.

Comment by Ghadi Shayban [ 10/May/16 12:56 PM ]

A few unsatisfactory things about overloading {iterate}
1) iterate's docstring says {f must be free of side-effects}
2) There is boilerplate and subtlety around the terminating item. In this case the final API call is made unconditionally, leading to an extra empty/marker item that is filtered by take-while. With the current proposal, the predicate controls iteration from the inside out

Comment by Ghadi Shayban [ 06/Jun/16 8:40 AM ]

updated patch to apply cleanly to core

Comment by Ghadi Shayban [ 18/Sep/16 11:40 PM ]

I'm not sure I'm sold on this anymore, and have suggested a different approach on the mailing list https://groups.google.com/d/msg/clojure-dev/89RNvkLdYc4/PAJh8gfmDAAJ

Comment by Ghadi Shayban [ 17/Apr/17 1:40 PM ]

I have been marinating upon two generator functions that cover use-cases including the one listed above.

One of them is similar to Scheme's unfold but with some deviations more appropriate to Clojure. The other function takes a side-effecting producer and a sentinel value.

Ignore the naming and examine the semantics. https://gist.github.com/ghadishayban/902373e247e920855139902912d237f0





[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-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-1243] Cannot resolve public generic method from package-private base class Created: 01/Aug/13  Updated: 14/Apr/17

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

Type: Defect Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: interop

Attachments: GZip Archive clj-1243-demo1.tar.gz     Text File invocation_target_selection.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The Clojure compiler cannot resolve a public generic method inherited from a package-private base class.

Instructions to reproduce:

  • In package P1
    • Define a package-private class A with generic type parameters
    • Define a public method M in A using generic types in either its arguments or return value
    • Define a public class B which extends A
  • In package P2
    • Construct an instance of B
    • Invoke B.M()

This is valid in Java. In Clojure, invoking B.M produces a reflection warning, followed by the error "java.lang.IllegalArgumentException: Can't call public method of non-public class." No amount of type-hinting prevents the warning or the error.

Attachment clj-1243-demo1.tar.gz contains sample code and script to demonstrate the problem.

Examples of Java projects which use public methods in package-private classes:



 Comments   
Comment by Stuart Sierra [ 01/Aug/13 5:11 PM ]

It is also not possible to call the method reflectively from Java.

This may be a bug in Java reflection: JDK-4283544

But why does it only happen on generic methods?

Comment by Stuart Sierra [ 08/Aug/13 11:59 AM ]

According to Rich Hickey, the presence of bridge methods is unspecified and inconsistent across JDK versions.

A possible solution is to use ASM to examine the bytecode of third-party Java classes, instead of the reflection API. That way the Clojure compiler would have access to the same information as the Java compiler.

Comment by Andy Fingerhut [ 17/Nov/13 11:01 PM ]

CLJ-1183 was closed as a duplicate of this one. Mentioning it here in case anyone working on this ticket wants to follow the link to it and read discussion or test cases described there.

Comment by Noam Ben Ari [ 21/Feb/15 4:55 AM ]

The current work around I use is to define a new Java class, add a static method that does what I need, and call that from Clojure.

Comment by Noam Ben Ari [ 21/Feb/15 9:28 AM ]

Also, I'm seeing this issue in 1.6 and 1.7(alpha5) but the issue mentions only up to 1.5 .

Comment by Adam Tait [ 03/Apr/16 5:32 PM ]

Just ran into this issue trying to use Google's Cloud APIs.
To use Google's Cloud Datastore, you need to access the .kind method on a protected generic subclass (BaseKey), to which KeyFactory extends.

Tested on both Clojure 1.7 & 1.8 at runtime, the following exception persists;

IllegalArgumentException Can't call public method of non-public class: public com.google.gcloud.datastore.BaseKey$Builder com.google.gcloud.datastore.BaseKey$Builder.kind(java.lang.String) clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:88)

Comment by Kai Strempel [ 18/Jun/16 1:19 PM ]

I ran into the exact same issue with Google's Cloud API's.

Tested it with 1.8 and with 1.9.0-alpha7. Same Problem.

Comment by Kai Strempel [ 18/Jun/16 1:19 PM ]

I ran into the exact same issue with Google's Cloud API's.

Tested it with 1.8 and with 1.9.0-alpha7. Same Problem.

Comment by Michal Růžička [ 23/Sep/16 1:08 PM ]

I ran into the same issue. The attached patch fixes the problem for me.
All tests in the project still pass, but this desperately needs a review of someone knowledgeable.

Comment by Alex Miller [ 23/Sep/16 3:00 PM ]

Hey Michal,

Thanks for looking at it.

1. Please follow the instructions on how to create a patch in the proper format here: http://dev.clojure.org/display/community/Developing+Patches
2. If you can provide some explanation of the changes to aid in review that would be most helpful. Otherwise screeners have to re-engineer your thought processes from scratch.
3. Before getting screened, this change will also need some tests (admittedly not particularly fun to write, but I think it's necessary here)

Comment by Michal Růžička [ 27/Sep/16 8:56 AM ]

I've added tests and updated the patch according to the instructions.

Here is some reasoning behind it. Below is an excerpt from the src/jvm/clojure/lang/Compiler.java file:

src/jvm/clojure/lang/Compiler.java
1462:	if(target.hasJavaClass() && target.getJavaClass() != null)
1463:		{
1464:		List methods = Reflector.getMethods(target.getJavaClass(), args.count(), methodName, false);
1465:		if(methods.isEmpty())
1466:			{
1467:			method = null;
1468:			if(RT.booleanCast(RT.WARN_ON_REFLECTION.deref()))
1469:				{
1470:				RT.errPrintWriter()
1471:					.format("Reflection warning, %s:%d:%d - call to method %s on %s can't be resolved (no such method).\n",
1472:						SOURCE_PATH.deref(), line, column, methodName, target.getJavaClass().getName());
1473:				}
1474:			}
1475:		else
1476:			{
1477:			int methodidx = 0;
1478:			if(methods.size() > 1)
1479:				{
1480:				ArrayList<Class[]> params = new ArrayList();
1481:				ArrayList<Class> rets = new ArrayList();
1482:				for(int i = 0; i < methods.size(); i++)
1483:					{
1484:					java.lang.reflect.Method m = (java.lang.reflect.Method) methods.get(i);
1485:					params.add(m.getParameterTypes());
1486:					rets.add(m.getReturnType());
1487:					}
1488:				methodidx = getMatchingParams(methodName, params, args, rets);
1489:				}
1490:			java.lang.reflect.Method m =
1491:					(java.lang.reflect.Method) (methodidx >= 0 ? methods.get(methodidx) : null);
1492:			if(m != null && !Modifier.isPublic(m.getDeclaringClass().getModifiers()))
1493:				{
1494:				//public method of non-public class, try to find a public descendant
1495:				if((type=Reflector.getDeepestPublicDescendant(m.getDeclaringClass(), target.getJavaClass())) == null)
1496:					//if descendant not found, try to find an ancestor
1497:					m = Reflector.getAsMethodOfPublicBase(m.getDeclaringClass(), m);
1498:				}
1499:			method = m;
1500:			if(method == null && RT.booleanCast(RT.WARN_ON_REFLECTION.deref()))
1501:				{
1502:				RT.errPrintWriter()
1503:					.format("Reflection warning, %s:%d:%d - call to method %s on %s can't be resolved (argument types: %s).\n",
1504:						SOURCE_PATH.deref(), line, column, methodName, target.getJavaClass().getName(), getTypeStringForArgs(args));
1505:				}
1506:			}
1507:		}
  • the condition on line 1462 ensures that the type/class of the target is known
  • the clojure.lang.Reflector.getMethods() method called on line 1464 returns a list of all public methods of the given name defined for the target type
  • then the best method to call is selected on lines 1477-1491
  • if the declaring class of the selected method is not public then an attempt is made to find a public class which is both superclass of the target type and a subclass of the class declaring the selected method - this is implemented in the clojure.lang.Reflector.getDeepestPublicDescendant() method
  • if such a class is found than it is used instead of the method's declaring class when emitting the byte code for the method call
  • if no such class is found then an attempt is made to find a compatible method in the public ancestors of the class declaring the selected method

Note that the change may result in a different method being called than prior to the change as demonstrated by the selecting-method-on-nonpublic-interface test. This is IMO an acceptable change as it:

  • results in better matching (with respect to the argument types) method to be called
  • makes the method selection in clojure behave in a more similar way to that in java
Comment by Stuart Sierra [ 14/Apr/17 4:37 PM ]

CLJ-126 describes a similar issue on Java 5.





[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: 13/Apr/17

Status: Open
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: Unresolved Votes: 0
Labels: docstring, spec

Attachments: Text File fixed-precision-doc-3.patch     Text File fixed-precision-doc.patch     Text File fixed-precision-doc.patch    
Patch: Code
Approval: Screened

 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: fixed-precision-doc-3.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.





[CLJ-1747] eduction's printer requires/promises that its source collection is Iterable Created: 07/Jun/15  Updated: 12/Apr/17

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

Type: Defect Priority: Minor
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJ-1747-eduction-print.patch    
Patch: Code

 Description   

eduction expects its source collection to be Iterable [1], and its print-method goes through print-sequential [2]. This implies a promise that may restrict the use-case of an eduction over a virtual collection, e.g. a pure IReduceInit source that may be backed by I/O or some other resource. I have found it useful to construct these I/O reducibles and wrap them with an eduction. But when interacting at the REPL, printing out an eduction-wrapped IReduceInit will fail. Does the print-method impl for eduction require too much? This is a only minor inconvenience more than anything else, obviously I could create my own flavor of eduction.

Totally hypothetical example:

(defn database-index
  [name]
  (reify clojure.lang.IReduceInit
    (reduce [_ f init]
      (with-open [rdr (fressian/create-reader (io/input-stream name))]
       (loop [] ...reduce impl...)))))

(eduction (filter (as-of #inst "2012-01-01")) (database-index "eavt.fress"))
;; ^ throws when printed by repl

[1] https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L7336-L7338
[2] https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L7360

Proposed Approach:
Since we have #object tagged literals, let eduction print like an opaque #object






[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: 0
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-2144] clojure.walk/keywordize-keys wants ns support for clojure.spec utility Created: 08/Apr/17  Updated: 11/Apr/17

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

Type: Enhancement 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    
Patch: Code and Test

 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-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-2141] New qualified-* predicates can return true, nil, and false Created: 31/Mar/17  Updated: 11/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: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 89
Labels: function

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

 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: In my opinion, the best combination of returning logical values, using a logical expression, and performance is:
(and (keyword? x) (not (nil? (namespace x))))

Patch: clj-2141.patch - implements the discussed approach



 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.





[CLJ-1435] 'numerator and 'denominator fail to handle integral values (i.e. N/1) Created: 30/May/14  Updated: 08/Apr/17

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

Type: Enhancement Priority: Major
Reporter: Aaron Brooks Assignee: Unassigned
Resolution: Unresolved Votes: 10
Labels: math

Attachments: Text File clj-1435.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

Because ratio values reduce to lowest terms and, for integral values where the lowest term is N/1, are auto-converted to BigInts (and formerly Longs), the current behavior of clojure.core/numerator and clojure.core/denominator yield unexpected results.

user=> (numerator 1/3)
1
user=> (numerator (+ 1/3 2/3))

ClassCastException clojure.lang.BigInt cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3306)
user=> (denominator 1/3)
3
user=> (denominator (+ 1/3 2/3))

ClassCastException clojure.lang.BigInt cannot be cast to clojure.lang.Ratio  clojure.core/denominator (core.clj:3314)
user=>

It's confusing to not support numerator and denominator on integer types as this requires you to always check ratio? before invoking them.

Proposed: Extend numerator and denominator to also work on integer types (long, BigInt, BigInteger) by routing to overloaded methods on Numbers for the desired types.

Patch: clj-1435.patch

Prescreening questions:

1. numerator and denominator are tagged as returning java.math.BigInteger (not clojure.lang.BigInt) and that's what I followed in the patch. Seems like maybe that should be BigInt though? Not sure on what basis to make that decision.
2. Should numerator and denominator accept both BigInteger and BigInt?



 Comments   
Comment by Andy Fingerhut [ 30/May/14 6:35 PM ]

I don't know the official stance on this ticket, but will add some notes.

Aaron, numerator and denominator are pretty clearly documented to work on Ratio types only.

It is pretty easy to write my-numerator and my-denominator that work exactly as you wish, checking for the type of arg and using numerator, denominator for Ratio types, and doing whatever you think is correct for other numeric types.

Comment by Aaron Brooks [ 30/May/14 7:44 PM ]

I'm aware that they are documented as such. Part of my point is that you can be working entirely with Ratio types and, via arithmetic operations between them, sometimes wind up with a non-Ratio number unexpectedly.

Also consider:

user=> (numerator 2/1)
ClassCastException java.lang.Long cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3238)

You're then left either implementing a try/catch correction or always checking the type before using numerator or denominator which is a loss in performance.

The patch I have in mind is creating a protocol, extended to Ratio, BigInt and Long which calls the appropriate method (Ratios) or returns either the given number or 1 (numerator/denominator) for the integral types. I expect this to maintain the current level of performance in the cases where it works and behave properly in the cases currently not handled.

Comment by Gary Fredericks [ 27/Aug/15 10:38 AM ]

I've definitely written the helper functions Andy describes on several occasions.

Comment by Felipe Micaroni Lalli [ 01/Sep/15 4:58 PM ]

Related issue: https://stackoverflow.com/questions/25194809/how-to-convert-any-number-to-a-clojure-lang-ratio-type-in-clojure

A workaround to that is (numerator (clojure.lang.Numbers/toRatio (rationalize <put any type of number here>)))

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

I agree with the intent of the ticket here that these should work. I'm not sure about the protocol approach as that would be an open system and I'm not sure that's what we want. An alternative would be to just create Java methods on Numbers that took the appropriate types and let the JVM sort it out.

Comment by Aaron Brooks [ 08/Apr/17 12:38 PM ]

Any progress on this?

Comment by Alex Miller [ 08/Apr/17 1:28 PM ]

Sitting in the queue, waiting for love.

Comment by Aaron Brooks [ 08/Apr/17 3:54 PM ]

Can I give it love? If there's a direction that we can agree on, I'd be happy to create the patch.

Comment by Alex Miller [ 08/Apr/17 6:13 PM ]

I've screeened the patch that's here already?





[CLJ-2046] generate random subsets of or'd required keys in map specs Created: 17/Oct/16  Updated: 07/Apr/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: David Chelimsky Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: generator, spec

Attachments: Text File clj-2046-2.patch     Text File map-spec-gen-enhancements.patch    
Patch: Code and Test
Approval: Screened

 Description   

(s/keys :req [(or ::x ::y)]) always generates maps with both ::x and ::y but it should also generate maps with either ::x or ::y.

The attached patch supports arbitrarily deeply nested or and and expressions within the values of :req and :req-un in map specs. It also uses the same 'or' mechanism for :opt and :opt-un keys, thereby replacing the use of clojure.core/shuffle with clojure.test.check.generators/shuffle, ensuring repeatability of the generators.

Patch: clj-2046-2.patch

Screened by: Alex Miller



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

Patch updated to apply to current master, no semantic changes, attribution retained.





[CLJ-2076] s/coll-of and s/map-of do not unform their elements Created: 05/Dec/16  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: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec

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

 Description   

s/coll-of and s/map-of unform with identity but should unform their elements:

(s/def ::o (s/coll-of (s/or :i integer? :s string?)))
(->> [1 2 "blah"] (s/conform ::o) (s/unform ::o))
=> [[:i 1] [:i 2] [:s "blah"]]

Expected: [1 2 "blah"]

Cause: every-impl unform* just returns x

Approach: Use the init/add/complete fns to generate an unformed value (when needed)

Patch: clj-2076-3.patch



 Comments   
Comment by Alex Miller [ 07/Dec/16 5:50 PM ]

This needs tests and a bunch of verification, but first pass at fixing.

Comment by Alex Miller [ 09/Dec/16 8:03 AM ]

Added tests, ready to screen

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

Added patch -3, only updated to apply cleanly to master.





[CLJ-2102] Reduce collection generator default size from 20 Created: 25/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: 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.patch    
Patch: Code
Approval: Vetted

 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-2.patch



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

Updated patch to apply to master





[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-2066] Reflection on internal classes fails under Java 9 Created: 22/Nov/16  Updated: 06/Apr/17

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

Type: Defect Priority: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: interop, reflection, regression

Attachments: Text File tcrawley.CLJ-2066.2017-03-16.patch    
Patch: Code and Test
Approval: Incomplete

 Description   

Due to changes in reflective access for the Jigsaw module system in Java 9, the Reflector will now fail on some cases that worked in previous Java versions.

(def fac (javax.xml.stream.XMLInputFactory/newInstance))
(.createXMLStreamReader fac (java.io.StringReader. ""))

Here fac will be an instance of com.sun.xml.internal.stream.XMLInputFactoryImpl, which is an implementation of javax.xml.stream.XMLInputFactory. In the new java.xml module, javax.xml.stream is an exported package, but the XMLInputFactoryImpl is an internal implementation of the public interface in that package. The invocation of createXMLStreamReader will be reflective and the Reflector will attempt to invoke the method based on the implementation class, which is not accessible outside the module, yielding:

IllegalAccessException class clojure.lang.Reflector cannot access class com.sun.xml.internal.stream.XMLInputFactoryImpl (in module java.xml) because module java.xml does not export com.sun.xml.internal.stream to unnamed module @4722ef0c
	jdk.internal.reflect.Reflection.throwIllegalAccessException (Reflection.java:423)
	jdk.internal.reflect.Reflection.throwIllegalAccessException (Reflection.java:414)
	jdk.internal.reflect.Reflection.ensureMemberAccess (Reflection.java:112)
	java.lang.reflect.AccessibleObject.slowCheckMemberAccess (AccessibleObject.java:632)
	java.lang.reflect.AccessibleObject.checkAccess (AccessibleObject.java:624)
	java.lang.reflect.Method.invoke (Method.java:539)
	clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:93)
	clojure.lang.Reflector.invokeInstanceMethod (Reflector.java:28)

One workaround here is to avoid the reflective call by type-hinting to the public exported interface:

(.createXMLStreamReader ^javax.xml.stream.XMLInputFactory fac (java.io.StringReader. ""))

Another (undesirable) workaround is to export the private package from java.xml to the unnamed module (which is the module used when code is loaded from the classpath rather than from a module) when invoking java/javac:

java --add-exports=java.xml/com.sun.xml.internal.stream=ALL-UNNAMED --add-exports=java.xml/com.sun.xml.internal.stream.writers=ALL-UNNAMED --add-exports=java.xml/com.sun.org.apache.xerces.internal.impl=ALL-UNNAMED ...

Proposed: The attached patch will check for and catch IllegalAccessException in the Reflector. When it occurs, the Reflector will attempt to invoke the method on all super-classes and super-interfaces in case one of them can succeed.

Patch: tcrawley.CLJ-2066.2017-02-14.patch

More info:



 Comments   
Comment by Toby Crawley [ 22/Nov/16 10:02 AM ]

This is the root cause of http://dev.clojure.org/jira/browse/DXML-32

Comment by Toby Crawley [ 14/Feb/17 3:48 PM ]

I don't see any indication that Java 9 will change in our favor, so I'm attaching a patch to fix this in Clojure itself.

Comment by Alex Miller [ 07/Mar/17 4:04 PM ]

Screening comments:

I think by just inspecting the patch that the new code will attempt to invoke the method on all super classes and interfaces, many of which will not have the applicable method and will thus throw IllegalArgumentException, possibly before finding the correct one. Can we reduce the effort here by only attempting the call on the super types that actually have the method? It would be good to expand the tests to also include this case if possible if I'm just mis-reading things (I did not attempt to construct this scenario).

Also, is there a method we can call to determine whether the method is accessible before invoking and needing to catch IllegalAccessException? (Obviously, we would want something that is not new in Java 9 so that this call worked on older JDKs too.)

Comment by Herwig Hochleitner [ 07/Mar/17 5:37 PM ]

This may be a stupid question, but has there been any research, whether [1] and [2] could be utilized to handle this without catching exceptions?

[1] http://download.java.net/java/jdk9/docs/api/java/lang/Class.html#getModule--
[2] http://download.java.net/java/jdk9/docs/api/java/lang/reflect/Module.html#isExported-java.lang.String-java.lang.reflect.Module-

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

Nope, that's the kind of thing I was asking about in my comment. However, those are jdk 9 only methods and right now Clojure supports jdk 6+. That's not impossible to deal with but does increase the difficulty.

Comment by Toby Crawley [ 08/Mar/17 2:30 PM ]

Alex: you are correct, an intermediate ancestor that does not provide the method will cause the lookup to fail - I'll rework and provide a new patch with better tests.

Herwig: we could build Clojure as a multi-release jar in order to have code that can use Java 9 features, but that would definitely complicate the build. However, there may be other Java 9 issues that may require us to take that option (I'm investigating one potential issue now).

Comment by Alex Miller [ 08/Mar/17 3:28 PM ]

Yeah, I was looking at the multi release jar stuff yesterday. I would really like to avoid that if at all possible as it adds a bunch of work for build, ci, and test.

Comment by Toby Crawley [ 13/Mar/17 9:42 PM ]

I've attached a new patch (tcrawley.CLJ-2066.2017-03-13.patch) and removed the old one (tcrawley.CLJ-2066.2017-02-14.patch). The new patch checks for any matching methods before calling invokeMatchingMethod; this will prevent the lack of the method on an ancestor from throwing IllegalArgumentException and aborting the lookup process.

I wasn't able to provide a test for the case where the method was missing on an ancestor, since that requires trying to invoke the method through an interface that doesn't provide it (all subclasses will provide methods from parent classes, so you need an ancestor tree that pulls in an interface), and I couldn't find anything in Java's stdlib that did that and was a non-exposed class. To implement a proper test for this, I believe we'd need to construct a module ourselves, which would require changes to the build process to build that module (and run the test) only under Java 9. I did, however, confirm locally that the old code was broken in this regard, and that the new patch fixes it.

Comment by Toby Crawley [ 14/Mar/17 12:00 PM ]

After reviewing tcrawley.CLJ-2066.2017-03-13.patch, I can see a new issue with it: if the method being invoked exists only on the non-public class, then the patch will throw an IllegalArgumentException claiming that the method doesn't exist instead of the original, more accurate IllegalAccessException. I'll rework and provide another patch.

Comment by Toby Crawley [ 16/Mar/17 9:22 AM ]

New patch attached (tcrawley.CLJ-2066.2017-03-16.patch) that will properly throw the original IllegalAccessException when the method only exists on the non-public class. Patch tcrawley.CLJ-2066.2017-03-13.patch has been deleted.

Comment by Ghadi Shayban [ 05/Apr/17 6:56 PM ]

In JDK9 public classes housed in a non-exported package are now inaccessible. Previously public classes might not be public any more. Clojure should link to them neither non-reflectively nor reflectively – perhaps a slightly larger adjustment to the compiler/reflector method lookup?

Comment by Herwig Hochleitner [ 06/Apr/17 8:07 AM ]

Ghadi: does that mean, that we can't even get at the Class object to call http://download.java.net/java/jdk9/docs/api/java/lang/Class.html#getModule-- ?

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

My understanding (which may be faulty) is that the new module system is independent from the classloader setup (although the base classloader hierarchy has changed a bit) and that reflection is still largely the same with respect to examining classes and methods. But you may run into issues with invoking constructors or methods that are not allowed according to access restrictions. I'm not sure if the ability to load classes is affected (but I didn't think so).





[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: OHTA Shogo 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-2077] Clojure can't be loaded from the boot classpath under java 9 Created: 06/Dec/16  Updated: 03/Apr/17

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

Type: Defect Priority: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: java9

Approval: Triaged

 Description   

As part of the changes for the jigsaw module system in Java 9, the
java packages available to the boot classloader are now a subset of
the the full java distribution. This means that classes loaded via the
boot classloader cannot access any classes outside of that subset.

The list of packages not visible to the boot classloader are:

java.activation
java.annotations.common
java.compact1
java.compact2
java.compact3
java.compiler
java.corba
java.scripting
java.se
java.se.ee
java.sql
java.sql.rowset
java.transaction
java.xml.bind
java.xml.ws
jdk.accessibility
jdk.charsets
jdk.crypto.ec
jdk.crypto.pkcs11
jdk.dynalink
jdk.jsobject
jdk.localedata
jdk.naming.dns
jdk.scripting.nashorn
jdk.xml.dom
jdk.zipfs
jdk.attach
jdk.compiler
jdk.hotspot.agent
jdk.internal.le
jdk.internal.opt
jdk.jartool
jdk.javadoc
jdk.jconsole
jdk.jdeps
jdk.jdi
jdk.jlink
jdk.jshell
jdk.jstatd
jdk.jvmstat

Clojure itself only uses one package on that list: java.sql. It is
used in clojure.instant to provide print-method and
print-dup implementations for java.sql.Timestamp, and in
clojure.core/resultset-seq.

This can be seen with (using Clojure 1.4.0 or higher, and a early-access build
of Java 9, most recently tested with 9-ea+147):

java -Xbootclasspath/a:clojure.jar clojure.main -e "(require 'clojure.instant)"

This affects any clojure-based tool that puts itself on the boot
classpath in order to gain a startup time boost (both lein
and boot are affected currently).



 Comments   
Comment by Toby Crawley [ 06/Dec/16 12:34 PM ]

More details on the underlying change that is triggering this are available at http://openjdk.java.net/jeps/261 (search for java.sql to find the relevant section).

Comment by Alex Miller [ 06/Dec/16 8:41 PM ]

Does this need to be a ticket here? Or is this really an issue for build tools?

Comment by Toby Crawley [ 08/Dec/16 4:30 PM ]

That depends on if we want using Clojure from the boot classpath to be an acceptable use case. If not, then I agree, it is just an issue for tooling.

Comment by Toby Crawley [ 09/Dec/16 2:21 PM ]

I realized today that this issue doesn't actually affect boot, since it doesn't use the bootclasspath. So lein is the only tooling I know of that is affected by this.

Comment by Ivan Pierre [ 12/Dec/16 4:59 AM ]

https://docs.oracle.com/javase/8/docs/api/java/sql/Timestamp.html
Would it be possible to use java.util.Date instead. Alas it's not possible to downcast :
Due to the differences between the Timestamp class and the java.util.Date class mentioned above, it is recommended that code not views Timestamp values generically as an instance of java.util.Date. The inheritance relationship between Timestamp and java.util.Date really denotes implementation inheritance, and not type inheritance.

Comment by Ivan Pierre [ 12/Dec/16 8:22 AM ]

The problem with Date is that it doesn't deal with nanoseconds. But Timestamp is created by a long giving the TimeDate value in milliseconds.
The use of setNano and getNano are only to store the SQL TIMESTAMP. Wouldn't it be better to deal with this value in another way?

The other way is to take just what we need from TimeStamp, and it's just a little thing, I'll try to compile with that to see if some other thing comes after...

Test code : https://gist.github.com/ivanpierre/b0ea937dac97d910a7c3c1e5774028e0

Comment by Ivan Pierre [ 12/Dec/16 1:13 PM ]

Ok, I pass to the GNU version of Timestamp. The code is neater. I mixed some of Sun's for more consistency. I dropped the string management of dates as Clojure will do it in clojure.instant.

It still works. I had a doubt...

If I type (clojure.lang.TimeStamp. 3678141) the response will be :
==> #inst "1970-01-01T01:01:18.141000000-00:00" with a nano of
141000000

But is if set nano to 1 : (.setNanos (clojure.lang.TimeStamp. 3678141) 1) the response is : #inst "1970-01-01T01:01:18.000000001-00:00"

This is correct, but it's a little disturbing to see my nice .141 disappear...

I put a fork on my GitHub. Last commit : https://github.com/ivanpierre/clojure/commit/749a0184ee7409290dad9ff353605fcaabd64f69

So, good, now pass to Leinigen...

Comment by Alex Miller [ 13/Dec/16 10:32 AM ]

I think the first question here is: do we expect that Clojure should be loadable from the bootclasspath?

While I realize this is a hack people use, my initial answer would be no (that was never a design constraint afaik). But will need to defer to Rich on that.

Comment by Ivan Pierre [ 13/Dec/16 10:41 AM ]

Well Leiningen do it, even with the lein repl. I test to see if TimeStamp is the only thing.
Alas Leinigen is not 1.9 compatible, so I have to go down to version 1.8 to make the tests. (problem of conflict between clojure.spec and hara library)
A funny thing would be to pass the whole Clojure test in bootstrap so we would know...

Comment by Ivan Pierre [ 13/Dec/16 1:06 PM ]

The java Timestamp could be directly integrated into clojure.instant as it's a new datatype. So no need to worry about copyright stuff, and integer it in a complete manner to accept SQL TIMESTAMP, and some correct protocols.
The worst is that looking across the DBs documentation doesn't help a lot and some are quite contradictory.

Comment by Phil Hagelberg [ 03/Apr/17 1:05 PM ]

> I think the first question here is: do we expect that Clojure should be loadable from the bootclasspath?

I don't know if it's the right thing for Clojure to be able to be loaded from the bootclasspath, but as an additional point of data: Leiningen takes 2.5 seconds to load on my machine from the bootclasspath on Java 8. Without the bootclasspath, it takes 3.4 seconds on Java 8 and 4.5 seconds on Java 9. (This is just for one Clojure instance, not launching a project subprocess.) So in this case it nearly doubles the time, which is consistently in the top 5 complaints about working with Clojure.

I would not be surprised if other Clojure-based tools (IDEs/editors perhaps) would want to use the bootclasspath but I don't have any data on that. I know there are people using the bootclasspath for production servers, but they probably wouldn't be as upset about adding a few seconds as people using it on their own machine.

I would be happy to write the few lines of code needed to defer all references to the inaccessible classes until runtime if it's decided that's the way to go.

Comment by Ghadi Shayban [ 03/Apr/17 3:13 PM ]

Phil - would you mind sharing your environment and testing code? I see a slowdown without bootclasspath but not nearly as dramatic as what you see. The Java 9 builds you are using probably have extra debugging enabled.





[CLJ-2142] Namespace map syntax prevents duplicate key check Created: 02/Apr/17  Updated: 03/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: Gregg Reynolds Assignee: Unassigned
Resolution: Unresolved Votes: 1
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: Vetted

 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-1965] clojure.spec/def should support an optional doc-string Created: 19/Jun/16  Updated: 03/Apr/17

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

Type: Enhancement Priority: Minor
Reporter: Alexander Kiel Assignee: Unassigned
Resolution: Unresolved Votes: 35
Labels: spec

Approval: Triaged

 Description   

Like clojure.core/def clojure.spec/def should support an optional doc string because one usually likes to describe specs in more detail as one could through keyword naming.



 Comments   
Comment by Moritz Heidkamp [ 03/Nov/16 4:23 PM ]

Building on this idea, I suggest to add first-class metadata support to registered specs and implement doc strings in terms of that (i.e. the same way as with vars).

Comment by Josh Brandoff [ 01/Apr/17 5:02 PM ]

Hi! Was just discussing the potential for a feature like this with a colleague. What's the current status? Was thinking of potentially working on it but wanted to get feedback and guidance from the community first.

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

We don't have a recommended approach to this yet so not looking for a patch at this time.





[CLJ-2085] Add additional info to explain-data to help explain printers Created: 15/Dec/16  Updated: 03/Apr/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: spec

Attachments: Text File explain-data.patch    
Patch: Code
Approval: Screened

 Description   

Right now, the explain-data provided to the explain printer function only has the list of problem data. There are many interesting things a printer could do with access to the root spec and value but those are not currently available.

Proposed: Provide these values in the explain-data map (as ::spec and ::value).

Patch: explain-data.patch



 Comments   
Comment by David Chelimsky [ 03/Apr/17 9:26 AM ]

We're using explain-data to generate non-dev-readable messages in a browser. Access to the root structure would be helpful in cases where understanding the output requires specific knowledge of Clojure's data structures e.g. when a spec for a collection of maps is used to validate a map, in which case the :val key is bound to a MapEntry.





[CLJ-2080] clojure.spec/every-kv does not work correctly on vectors Created: 08/Dec/16  Updated: 31/Mar/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: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

alpha 14


Attachments: Text File clj-2080-2.patch     Text File clj-2080-3.patch     Text File clj-2080-4.patch     Text File clj-2080.patch    
Patch: Code
Approval: Vetted

 Description   

The every-kv doc states "takes separate key and val preds and works on associative collections". Vectors return true for associative? but do not currently work:

user=> (s/conform (s/every-kv any? any?) [])
[]
user=> (s/conform (s/every-kv any? any?) [1 2 3])
:clojure.spec/invalid
user=> (s/conform (s/every-kv integer? string?) [])
[]
user=> (s/conform (s/every-kv integer? string?) ["x"])
:clojure.spec/invalid

Another similar problem:

(s/explain-data (s/every-kv int? int?) [{:a :b}])
UnsupportedOperationException nth not supported on this type: PersistentArrayMap  clojure.lang.RT.nthFrom (RT.java:903)

Cause: The combination of every-kv and every-impl assume that the collection passed to every-kv can provide a seq of map entries (which is the intention). In the explain case, the ::kfn created by every-kv is used to create a better path segment using the key rather than the element index. The kfn assumes that an element of the collection can call `(nth entry 0)` on the element. In the explain failure above, the map {:a :b} will throw when invoked with nth.

Proposed: Do the following to be clearer about the requirement that the coll elements are map entries:

  • Modify the docstring to say "seqs to map entries" rather than "is a map"
  • Modify the kfn to add a check that the element is an entry and if so, use it's key. If not, use the index (of the element itself). In this case, when passed an entry it will report with the actual key but when passed something that's not an entry, it will report the collection based index of the non-entry.

Patch: clj-2080-4.patch



 Comments   
Comment by Alex Miller [ 09/Dec/16 8:35 AM ]

At the moment, I'm inclined to say the doc in every-kv should be tightened to say "map" instead of "associative collection" but will check with Rich.

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

Updated to apply to master





[CLJ-1714] Some static initialisers still run at compile time if used in type hints Created: 22/Apr/15  Updated: 29/Mar/17

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

Type: Defect Priority: Major
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler, typehints

Attachments: Text File clj-1714-4.patch     Text File CLJ-1714.patch     Text File CLJ-1714-v2.patch     Text File CLJ-1714-v3.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

AOT compiling on an x86 machine to be run on an ARM machine when a Java dependency has a native component and the class with the native dependency is used in a type hint.

In this situation, the only native library available on the classpath is the ARM dependency, and obviously won't load on the compiling x86 machine. Java libraries tend to load the native dependencies in the static initialiser of the class, which will fail in this situation as the architecture is x86 and the dependencies are ARM, for which reason CLJ-1315 made the change to not run static initialisers at compile time.

This covers a case which didn't come up as part of CLJ-1315, that the same problem occurs if rather than constructing the class, you simply use it as a type hint (which IMO is doubly surprising as something to have a side-effect).

Approach: Don't cause class to load merely from being in a type hint.

Patch: clj-1714-4.patch

This patch has been used in production for over a year with no adverse effects (as has anybody using the clojure-android build of clojure).

Prescreened: Alex Miller



 Comments   
Comment by Alex Miller [ 22/Apr/15 10:53 AM ]

I think this might have been logged already but I'm not sure.

Comment by Michael Blume [ 22/Apr/15 12:30 PM ]

Patch won't apply to master for me

Comment by Adam Clements [ 22/Apr/15 2:39 PM ]

Really sorry, don't know what happened there. I checked out a fresh copy of the repo and re-applied the changes, deleted the old patch as it was garbage. Try the new one, timestamped 2:37pm

Comment by Stuart Halloway [ 30/Jul/15 1:52 PM ]

Please add an example of the problem, and if possible a failing test.

Comment by Alex Miller [ 30/Jul/15 5:14 PM ]

Reset to "Open" as moving from Triaged->Incomplete is not valid in our current workflow.

Comment by Adam Clements [ 31/Jul/15 10:56 AM ]

Example problem:
AOT compiling on an x86 machine to be run on an ARM machine when a Java dependency has a native component and the class with the native dependency is used in a type hint.

In this situation, the only native library available on the classpath is the ARM dependency, and obviously won't load on the compiling x86 machine. Java libraries tend to load the native dependencies in the static initialiser of the class, which will fail in this situation as the architecture is x86 and the dependencies are ARM, for which reason CLJ-1315 made the change to not run static initialisers at compile time.

This covers a case which didn't come up as part of CLJ-1315, that the same problem occurs if rather than constructing the class, you simply use it as a type hint (which IMO is doubly surprising as something to have a side-effect).

This patch fixes that - happy to try and create a test, but would appreciate some advice on the shape such a test would take - presumably loading a java native library would be undesirable. I could simply check for static initialisers being run, but first would need some agreement that this is universally undesirable at compile time.

I have been using this patch in production for over a year with no adverse effects (as has anybody using the clojure-android build of clojure).

Comment by Stuart Halloway [ 31/Jul/15 11:34 AM ]

Hi Adam,

Thanks for the quick response. I think checking for static initializers being run is OK for a test.

Comment by Adam Clements [ 12/Aug/15 9:12 AM ]

Added failing tests which now pass

Comment by Michael Blume [ 28/Sep/16 2:07 PM ]

Updated patch to apply to master

Comment by Alex Miller [ 28/Sep/16 2:40 PM ]

Added new patch that is semantically identical, just easier to read (with formatted using -U8). Attribution retained.





[CLJ-840] Add a way to access the current test var in :each fixtures for clojure.test Created: 21/Sep/11  Updated: 28/Mar/17

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

Type: Enhancement Priority: Major
Reporter: Hugo Duncan Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: clojure.test

Attachments: File add-test-var.diff     File clj840-20161122.diff     File clj840-2.diff    
Patch: Code
Approval: Triaged

 Description   

When looking at (log) output from tests written with clojure.test, I would like to be able to identify the output associated with each test. A mechanism to expose the current test var within an :each fixture would enable this.

One mechanism might be to bind a test-var var with the current test var before calling the each-fixture-fn in clojure.test/test-all-vars.

Proposed: The proposed patch (clj840-20161122.diff) allows 'each' fixtures to access the var associated with the currently executing test by using (first *testing-vars*). As a result of this change, 'each' fixtures are able to access the metadata associated with the current test var, including the name.

The patch achieves the above by changing the order in which functions are wrapped when a test and its associated 'each' fixtures are run. Before this patch, 'each' fixtures were combined into a single higher-order function, which was then given a thunk containing an invocation of the test-var function to execute as its body. After this patch, the test-var function is now responsible for joining and executing 'each' fixtures but, importantly, it does so within the scope of the binding expression that adds the current test var to *testing-vars*. test-var now invokes the joined fixtures function, rather than the joined fixtures function being given a thunk that invokes test-var.

Patch: clj840-20161122.diff



 Comments   
Comment by Stuart Sierra [ 07/Oct/11 4:33 PM ]

Or just pass the Var directly into the fixture. Vars are invokable.

Comment by Hugo Duncan [ 07/Oct/11 4:45 PM ]

I don't think that works, since the the function passed to the fixture is not the test var, but a function calling test-var on the test var.

Comment by Hugo Duncan [ 21/Oct/11 10:34 PM ]

Patch to add test-var

Comment by Stuart Sierra [ 25/Oct/11 6:04 PM ]

*testing-vars* already has this information, but it's not visible to the fixture functions because it gets bound inside test-var.

Perhaps the :each fixture functions should be called in test-var rather than in test-all-vars. (The namespace of a Var is available in its metadata.) But then we have to call join-fixtures inside test-var every time.

Comment by Stuart Sierra [ 25/Oct/11 6:26 PM ]

Try this patch: clj840-2.diff.

This makes *testing-vars* visible to :each fixture functions, which seems intuitively more correct.

BUT it slightly changes the behavior of test-var, which I'm less happy about.

Comment by Hugo Duncan [ 25/Oct/11 8:07 PM ]

Might it make sense to provide a function on top of testing-vars to return the current test-var?

Comment by Stuart Sierra [ 28/Oct/11 9:14 AM ]

No, that function is first

Comment by Hugo Duncan [ 28/Oct/11 11:31 AM ]

I agree with having the dynamic vars as part of the extension interface, but would have thought that having a function for use when writing tests would have been cleaner. Just my 2c.

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

With a commit made on Nov 22, 2013, patch clj840-2.diff no longer applies cleanly to latest master. Updating it appears like it might be straightforward, but best for someone who knows this part of the code well to do so.

Comment by Joe Littlejohn [ 22/Nov/16 11:30 AM ]

I'd find it very useful if this one was fixed.

I've added an updated patch that has the same content as clj840-2.diff but applies against the current master (c0326d2), as of November 22nd 2016.

Comment by Joe Littlejohn [ 28/Nov/16 5:59 AM ]

I realise I've only translated a patch provided by someone else here, but if there's anything further you think this one needs before it's in a fit state to be considered then please do shout and I'll endeavour to add something further. Thanks.

Comment by Alex Miller [ 29/Nov/16 8:56 AM ]

If you could update the ticket to better describe the approach of the patch that would be helpful.

Comment by Joe Littlejohn [ 02/Dec/16 9:08 AM ]

The proposed patch (clj840-20161122.diff) allows 'each' fixtures to access the var associated with the currently executing test by using (first *testing-vars*). As a result of this change, 'each' fixtures are able to access the metadata associated with the current test var, including the name.

The patch achieves the above by changing the order in which functions are wrapped when a test and its associated 'each' fixtures are run. Before this patch, 'each' fixtures were combined into a single higher-order function, which was then given a thunk containing an invocation of the test-var function to execute as its body. After this patch, the test-var function is now responsible for joining and executing 'each' fixtures but, importantly, it does so within the scope of the binding expression that adds the current test var to *testing-vars*. test-var now invokes the joined fixtures function, rather than the joined fixtures function being given a thunk that invokes test-var.

Hopefully that's clear, ish

Comment by Joe Littlejohn [ 23/Mar/17 11:42 AM ]

Any chance this one could be considered for inclusion now? The file clj840-20161122.diff is still valid and can be applied to the current HEAD of master.

Comment by Alex Miller [ 23/Mar/17 2:56 PM ]

Can you check that changing the function ordering here hasn't affected the stack trace inspection in do-report? Basically it would be helpful to know in all the various cases (neither each nor once, each but not once, once but not each, and both) that error reporting is unaffected for all those cases. Also, tools like Leiningen monkeypatch parts of this as well. I think all of that's fine, but I'm probably not going to get around to checking all that soon.

Comment by Joe Littlejohn [ 28/Mar/17 3:26 AM ]

Thanks for taking the time to think about that todo list, Alex. Much appreciated. I'll investigate these things.





[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-1381] Improve support for extending protocols to primitive arrays Created: 13/Mar/14  Updated: 18/Mar/17

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: protocols

Approval: Vetted

 Description   

It is possible to extend protocols to primitive arrays but specifying the class for the type is a little tricky:

(defprotocol P (p [_]))
(extend-protocol P (Class/forName "[B") (p [_] "bytes"))
(p (byte-array 0))   ;; => "bytes"

However, things go bad if you try to do more than one of these:

(extend-protocol P 
  (Class/forName "[B") (p [_] "bytes")
  (Class/forName "[I") (p [_] "ints"))
CompilerException java.lang.UnsupportedOperationException: nth not supported on this type: Character, compiling:(NO_SOURCE_PATH:1:1)
	clojure.lang.Compiler.analyze (Compiler.java:6380)
	clojure.lang.Compiler.analyze (Compiler.java:6322)
	clojure.lang.Compiler$MapExpr.parse (Compiler.java:2879)
	clojure.lang.Compiler.analyze (Compiler.java:6369)
	clojure.lang.Compiler.analyze (Compiler.java:6322)
	clojure.lang.Compiler$InvokeExpr.parse (Compiler.java:3624)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6562)
	clojure.lang.Compiler.analyze (Compiler.java:6361)
	clojure.lang.Compiler.analyze (Compiler.java:6322)
	clojure.lang.Compiler$BodyExpr$Parser.parse (Compiler.java:5708)
	clojure.lang.Compiler$FnMethod.parse (Compiler.java:5139)
	clojure.lang.Compiler$FnExpr.parse (Compiler.java:3751)
Caused by:
UnsupportedOperationException nth not supported on this type: Character
	clojure.lang.RT.nthFrom (RT.java:857)
	clojure.lang.RT.nth (RT.java:807)
	clojure.core/emit-hinted-impl/hint--5951/fn--5953 (core_deftype.clj:758)
	clojure.core/map/fn--4207 (core.clj:2487)
	clojure.lang.LazySeq.sval (LazySeq.java:42)
	clojure.lang.LazySeq.seq (LazySeq.java:60)
	clojure.lang.RT.seq (RT.java:484)
	clojure.lang.RT.countFrom (RT.java:537)
	clojure.lang.RT.count (RT.java:530)
	clojure.lang.Cons.count (Cons.java:49)
	clojure.lang.Compiler.analyze (Compiler.java:6352)
	clojure.lang.Compiler.analyze (Compiler.java:6322)

The code in {parse-impls} is seeing the second {(Class/forName "[I")} as a function, not as a new type. One workaround for this is to only extend the protocol to one type at a time.

It would be even better (moving into enhancement area) if there was a syntax here to specify primitive array types - we already have the syntax of {bytes, ints, longs}, etc for type hints and those seem perfectly good to me.



 Comments   
Comment by Nahuel Greco [ 18/Sep/14 6:08 PM ]

It also breaks when extending only one array type:

(extend-protocol P
  String               (p [_] "string")
  (Class/forName "[B") (p [_] "ints") 
  )

;=> CompilerException java.lang.UnsupportedOperationException: nth not supported on this type ...

But changing the declaration order fixes it:

(extend-protocol P
  (Class/forName "[B") (p [_] "ints") 
  String               (p [_] "string")
  )

;=> OK
Comment by Alex Miller [ 12/Jan/16 3:16 PM ]

Dupe of CLJ-1790

Comment by Alex Miller [ 07/Jun/16 4:00 PM ]

On further inspection, I don't think this is a dupe of CLJ-1790 but merely a related problem.

Comment by Greg Chapman [ 18/Mar/17 11:04 AM ]

Using Class/forName has a further problem, as type-hints on the this parameter are longer emitted:

user=> (extend-protocol P (Class/forName "[B") (p [this] (aget this 0)))
Reflection warning, NO_SOURCE_PATH:2:50 - call to static method aget on clojure.lang.RT can't be resolved (argument types: unknown, int).

Reflection warnings are also generated for non-primitive arrays (so just supporting ints etc, won't completely fix this problem). It would be good to have a solution which covered all the problems with extending protocols to arrays.





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

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 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: 16/Mar/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: 1
Labels: error-reporting, regression, spec
Environment:

1.9.0-alpha12-1.9.0-alpha15


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

 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-1544] AOT bug involving namespaces loaded before AOT compilation started Created: 01/Oct/14  Updated: 14/Mar/17

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

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

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

 Description   

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

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

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

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

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

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

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

Screened by: Alex Miller



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

Possibly related: CLJ-1457

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

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

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

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

>cat error.sh
#!/bin/sh

rm -rf target && mkdir target

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

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

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

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

> ./error.sh
Exception in thread "main" java.lang.ExceptionInInitializerError
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.classForName(RT.java:2113)
	at clojure.lang.RT.classForName(RT.java:2122)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5403.invoke(core.clj:5808)
	at clojure.core$load.doInvoke(core.clj:5807)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5352.invoke(core.clj:5653)
	at clojure.core$load_lib.doInvoke(core.clj:5652)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5691)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:630)
	at clojure.core$use.doInvoke(core.clj:5785)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at user$eval212.invoke(NO_SOURCE_FILE:1)
	at clojure.lang.Compiler.eval(Compiler.java:6767)
	at clojure.lang.Compiler.eval(Compiler.java:6730)
	at clojure.core$eval.invoke(core.clj:3076)
	at clojure.main$eval_opt.invoke(main.clj:288)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.io.FileNotFoundException: Could not locate myrecord__init.class or myrecord.clj on classpath.
	at clojure.lang.RT.load(RT.java:443)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5403.invoke(core.clj:5808)
	at clojure.core$load.doInvoke(core.clj:5807)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5352.invoke(core.clj:5653)
	at clojure.core$load_lib.doInvoke(core.clj:5652)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5691)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5774)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at core__init.load(Unknown Source)
	at core__init.<clinit>(Unknown Source)
	... 33 more

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

This way, both under JIT and AOT code like

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

will fail with a circular depdenency error.

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

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

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

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

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

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

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

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

Comment by John Szakmeister [ 27/Oct/16 4:54 AM ]

I just spent quite a bit of time tracking down what I thought might be a variant of this problem. I've been trying to use Colin Fleming's new gradle-clojure plugin and was running into issues with the resulting shadow jar (the equivalent of an uberjar). At the time I believed it to be a problem in the gradle-clojure plugin, but it turned out to be a different issue. The shadow plugin was not preserving the last modified time on files extracted from dependencies, and it resulted in some source files looking newer than the class files. I suspect that Clojure was then recompiling the class, thinking it was out-of-date. This was nasty to track down and quite unexpected, but I can see the sense in the behavior now that I know what's going on. I'm not sure if anything should be done on the Clojure side, but it points to a problem with including the source and AOT files together--you really need to make sure the timestamps are kept intact and I can see that fact being easily overlooked. In this case, it was a plugin completely unrelated to Clojure that had to be fixed. I should also add that taking the infectious approach Stuart mentions is probably an issue in the Gradle and (possibly?) Maven environments, since there are separate plugins for packaging the uberjar.

FWIW, I have a fair amount of information in the ticket for gradle-clojure about the failure mode and the steps I went through to try and track down the problem: https://github.com/cursive-ide/gradle-clojure/issues/8. Also, I've put a pull request in on the shadow plugin to help keep the timestamps intact: https://github.com/johnrengelman/shadow/pull/260. There is also an issue in the shadow plugin describing the problem too: https://github.com/johnrengelman/shadow/issues/259.

Comment by Mike Rodriguez [ 14/Mar/17 6:15 PM ]

I'm confused by http://dev.clojure.org/jira/browse/CLJ-1544?focusedCommentId=36734&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-36734

Isn't the issue just that the "clj" source files also need to be included on the runtime classpath in order to be JITed when needed - since they were not AOTed?

i.e.

cp src/myrecord.clj target/

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

Why would the expectation be that "target" contains everything needed by the classpath when the AOT compilation was not done on everything? I don't get a failure when I add this step here.

The AOTed code looks fine. It just calls a `require` on the `myrecord` ns to JIT it. So I'm guessing I'm not seeing something else about this general problem statement.





[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-1793] Reducer instances hold onto the head of seqs Created: 05/Aug/15  Updated: 14/Mar/17

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

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

1.8.0-alpha2 - 1.8.0-alpha4


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

 Description   

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

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

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

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

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

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

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

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

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

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

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

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

Patch: clj-1793-3.patch

Screened by: Alex Miller



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

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

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

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

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

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

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

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

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

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

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

See the commit message for an extensive-ish comment.

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

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

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

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

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

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

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

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

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

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

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

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

Before patch:

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

After patch:

user=> (defn range* [x] (cons x (lazy-seq (range* (inc x)))))
#'user/range*
user=> (reduce + 0 (eduction (range* 0)))
;; runs infinitely without causing OOM
Comment by Alex Miller [ 08/Sep/16 5:14 PM ]

Refreshed patch to apply to master. No semantic changes, attribution retained.

Comment by Alex Miller [ 14/Mar/17 11:42 AM ]

Looks like this one was missed when applying patches for release





[CLJ-1743] Avoid compile-time static initialization of classes when using inheritance Created: 02/Jun/15  Updated: 13/Mar/17

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

Type: Enhancement Priority: Critical
Reporter: Abe Fettig Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: aot, compiler, interop

Attachments: Text File 0001-Avoid-compile-time-class-initialization-when-using-g.patch     Text File clj-1743-2.patch     Text File clj-1743-3.patch    
Patch: Code
Approval: Triaged

 Description   

I'm working on a project using Clojure and RoboVM. We use AOT compilation to compile Clojure to JVM classes, and then use RoboVM to compile the JVM classes to native code. In our Clojure code, we call Java APIs provided by RoboVM, which wrap the native iOS APIs.

But we've found an issue with inheritance and class-level static initialization code. Many iOS APIs require inheriting from a base object and then overriding certain methods. Currently, Clojure runs a superclass's static initialization code at compile time, whether using ":gen-class" or "proxy" to create the subclass. However, RoboVM's base "ObjCObject" class [1], which most iOS-specific classes inherit from, requires the iOS runtime to initialize, and throws an error at compile time since the code isn't running on a device.

CLJ-1315 addressed a similar issue by modifying "import" to load classes without running static initialization code. I've written my own patch which extends this behavior to work in ":gen-class" and "proxy" as well. The unit tests pass, and we're using this code successfully in our iOS app.

Patch: clj-1743-2.patch

Here's some sample code that can be used to demonstrate the current behavior (Full demo project at https://github.com/figly/clojure-static-initialization):

Demo.java
package clojure_static_initialization;

public class Demo {
  static {
    System.out.println("Running static initializers!");
  }
  public Demo () {
  }
}
gen_class_demo.clj
(ns clojure-static-initialization.gen-class-demo
  (:gen-class :extends clojure_static_initialization.Demo))
proxy_demo.clj
(ns clojure-static-initialization.proxy-demo)

(defn make-proxy []
  (proxy [clojure_static_initialization.Demo] []))

[1] https://github.com/robovm/robovm/blob/master/objc/src/main/java/org/robovm/objc/ObjCObject.java



 Comments   
Comment by Alex Miller [ 18/Jun/15 3:01 PM ]

No changes from previous, just updated to apply to master as of 1.7.0-RC2.

Comment by Alex Miller [ 18/Jun/15 3:03 PM ]

If you had a sketch to test this with proxy and gen-class, that would be helpful.

Comment by Abe Fettig [ 22/Jun/15 8:31 AM ]

Sure, what form would you like for the sketch code? A small standalone project? Unit tests?

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

Just a few lines of Java (a class with static initializer that printed) and Clojure code (for gen-class and proxy extending it) here in the test description that could be used to demonstrate the problem. Should not have any dependency on iOS or other external dependencies.

Comment by Abe Fettig [ 01/Jul/15 8:49 PM ]

Sample code added, let me know if I can add anything else!

Comment by Abe Fettig [ 27/Jul/15 2:21 PM ]

Just out of curiosity, what are the odds this could make it into 1.8?

Comment by Alex Miller [ 27/Jul/15 6:06 PM ]

unknown.

Comment by Didier A. [ 20/Nov/15 7:11 PM ]

I'm affected by this bug too. A function in a namespace calls a static Java variable which is initialized in place. Another namespace which is genclassed calls that function. Now at compile time, the static java is initialized and it makes building fail, because that static java initialization needs resources which don't exist on the build machine.

Comment by Michael Blume [ 13/Mar/17 10:00 PM ]

Refreshing patch so it applies to master, no changes, keeping attribution.





[CLJ-994] repeat reducer Created: 11/May/12  Updated: 12/Mar/17

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

Type: Enhancement Priority: Major
Reporter: Jason Jackson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reducers

Attachments: Text File 0001-repeat-for-clojure.core.reducers.patch    
Patch: Code and Test

 Description   

i'm working on clojure.core/repeat reducer.



 Comments   
Comment by Andy Fingerhut [ 17/May/12 6:18 PM ]

Jason, have you tried to build this using JDK 1.6.0? I've tried on Mac OS X 10.6.8 + Oracle/Apple JDK 1.6.0 and Ubuntu 11.10 + IBM JDK 1.6.0, and on both it compiles, but during the tests fails with a ClassNotFoundException for class jsr166y.ForkJoinTask.

It builds and tests cleanly on Ubuntu 11.10 + Oracle JDK 1.7.0 for me.

Comment by Jason Jackson [ 17/May/12 6:41 PM ]

That's an issue that applies to all of core.reducers. Alan Malloy experienced it as well. I tried fixing it, but eventually just upgraded to JDK 1.7. I don't understand why it's happening.

Comment by Jason Jackson [ 19/May/12 2:55 PM ]

This issue is isolated to mvn test afaik.

When I include clojure inside a leiningen project, and add jsr166y.jar to lib directory, core.reducers works fine with java 1.6.

Comment by Andy Fingerhut [ 20/May/12 3:00 AM ]

Jason, you say it applies to all of core.reducers in your May 17, 2012 comment. I don't understand. Without your patch applied, I can run "./antsetup.sh ; ant" in a freshly-pulled Clojure git repo on either of the JDK 1.6.0 versions mentioned in my earlier comment, and do not get any errors during the tests. Are you saying perhaps that core.reducers currently has no tests that exercise the problem now, but your patch adds such tests that fail, even with no other changes to the code?

Comment by Jason Jackson [ 20/May/12 11:55 AM ]

Yah that's right. Now that you mention it, my patch is the first unit test to call r/fold (the existing tests do non-parallel reductions).

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

With Stuart Halloway's commit to Clojure master on June 8, 2012 titled "let reducers tests work under ant", patch 0001-repeat-for-clojure.core.reducers.patch dated May 11, 2012 now runs correctly even the new unit tests requiring class jsr166y.ForkJoinTask with Oracle/Apple JDK 1.6 and Linux IBM JDK 1.6.

Comment by Jason Jackson [ 14/Aug/12 1:17 AM ]

I'm on the contributors list. Is this patch still needed?
sorry for long long delay.

Comment by Jason Jackson [ 14/Sep/12 2:37 PM ]

This patch should wait until http://dev.clojure.org/jira/browse/CLJ-993 is committed. I think there's a some shared code.

Comment by Ghadi Shayban [ 09/Jan/16 5:24 PM ]

repeat is now reducible in 1.7.0

Comment by Ghadi Shayban [ 29/Jun/16 9:05 PM ]

Can we close this ticket?

Comment by Alex Gunnarson [ 12/Mar/17 9:49 PM ]

I think it would be nice to have this reducer included for purposes of parallelism, or else extend `CollFold` to `clojure.lang.Repeat` (which is perhaps cleaner).





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

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

Type: Enhancement Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: 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-1860] 0.0 and -0.0 compare equal but have different hash values Created: 01/Dec/15  Updated: 10/Mar/17

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

Type: Defect Priority: Minor
Reporter: Patrick O'Brien Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: math

Attachments: Text File clj-1860-2.patch     Text File clj-1860-make-equals-false-for-pos-neg-0.0-v1.patch     Text File CLJ-1860-negative-zero-hash-eq-fix.patch    
Patch: Code and Test
Approval: Screened

 Description   

0.0 and -0.0 compare as equal but have different hash values:

user=> (= 0.0 -0.0)
true
user=> (hash -0.0)
-2147483648
user=> (hash 0.0)
0

This causes problems as the equality/hashing assumption is violated.

user=> #{[1 2 0.0] [1 2 -0.0]}
#{[1 2 -0.0] [1 2 0.0]}

user=> (hash-map 0.0 1 -0.0 2)
{0.0 2}

user=> (hash-map [0.0] 1 [-0.0] 2)
{[0.0] 1, [-0.0] 2}

user=> (array-map [0.0] 1 [-0.0] 2)
{[0.0] 2}

user=> (hash-set [0.0] [-0.0])
#{[0.0] [-0.0]}

Cause: The source of this is due to some differences in Java. Java primitive double 0.0 and -0.0 == but the boxed Double is NOT .equals(). See also: http://docs.oracle.com/javase/7/docs/api/java/lang/Double.html#equals%28java.lang.Object%29

Double equality is checked with == in Clojure, which will report true. Hashing falls through to .hashCode(), which returns different values (but is consistent with the .equals() result on the boxed form).

Approach: While there are times when 0.0 and -0.0 being different are useful (see background below), most Clojure users expect these to compare equal. IEEE 754 says that they should compare as equals as well. So the approach to take here is to leave them as equal but to modify the hash for -0.0 to be the same as 0.0 so that `=` and `hash` are consistent. The attached patch takes this approach.

Patch: clj-1860-2.patch

Screened: Alex Miller

Alternative: Make 0.0 != -0.0. This approach affects a much larger set of code as comparison operators etc may be affected. The patch clj-1860-make-equals-false-for-pos-neg-0.0-v1.patch may be one way to implement this approach, and seems fairly small in the quantity of code affected (2 methods).

Background: https://en.wikipedia.org/wiki/Signed_zero



 Comments   
Comment by Stephen Hopper [ 09/Feb/16 10:45 PM ]

Just to summarize, it seems like this functionality in Clojure is the same as it is in Java:

0.0 == -0.0: true
new Double(0.0).hashCode(): 0
new Double(-0.0).hashCode(): -2147483648
new Double(-0.0).equals(new Double(0.0)): false

I can see pros and cons to both of the aforementioned approaches, as well as just leaving this one be. Does anyone else have any input on this one? Is this issue something we should rectify, or by changing it will we end up creating more problems than we solved?

Comment by Andy Fingerhut [ 10/Feb/16 12:38 AM ]

The Java behavior you demonstrate shows that in this case, they are not equals, so there is no need for the hashCode() values to be the same in order to satisfy the hash consistency property of equals and hashCode.

Clojure currently violates the hash consistency property that should ideally hold between clojure.core/= and clojure.core/hash, for 0.0 and -0.0.

Changing clojure.core/= so it is false would restore the hash consistency property for these values. Keeping (clojure.core/== 0.0 -0.0) true is hopefully something that will be maintained across any change, but that does not violate hash consistency, because that property has nothing to say about the value of clojure.core/==

Comment by Stephen Hopper [ 10/Feb/16 7:10 AM ]

Thanks for the explanation, Andy. That makes sense to me. I'll put together some tests and a possible solution for evaluation.

Comment by Stephen Hopper [ 10/Feb/16 11:20 AM ]

After diving into the source code a bit more, my preference is to modify the hash calculation for -0.0 to be the same as the hash calculation for 0.0. This will restore the hash consistency property without breaking other mathematical operations. Basically, if we update clojure.core/= to return false for (= 0.0 -0.0), we will need to update other functions (clojure.core/<, clojure.core/>, etc.) so that -0.0 and 0.0 still follow basic numerical properties surrounding equality and ordering. I'll add some tests and a possible patch to hash calculation for numbers for consideration.

Comment by Andy Fingerhut [ 10/Feb/16 12:40 PM ]

Someone, perhaps Patrick O'Brien , brought up a preference that making (= 0.0 -0.0) false would help in some applications, e.g. numerical applications involving normal vectors where it was beneficial if (= 0.0 -0.0) was false. I have no knowledge whether this preference will determine what change will be made to Clojure, if any. If someone finds a link to the email discussion that was in one of the Clojure or Clojure Dev Google groups, that would be a useful reference.

Comment by Stephen Hopper [ 11/Feb/16 9:47 PM ]

I've added a pair of patches for review on this one (one contains test updates and the other contains my proposed updates to the hash calculation). I realize that this is different than the approach proposed earlier in the ticket, but I think it should be the preferred approach. As I mentioned earlier, merely changing clojure.core/= to return false for (= 0.0 -0.0) would require also updating other numeric equality functions (clojure.core/<, clojure.core/>, etc.). More importantly, I feel that this behavior would be different than what most Clojure developers would expect.

For this reason, the patches I've updated merely modify the calculation of hashes with Numbers.java for Floats and Doubles for which isZero returns true to return the hashCode for positive 0.0 instead of negative 0.0.

Is this acceptable?

EDIT:
With these patches applied, we get the following behavior in the REPL:

user=> (= 0.0 -0.0)
true
user=> (hash 0.0)
0
user=> (hash -0.0)     
0
user=> (hash (float 0.0))
0
user=> (hash (float -0.0))
0
Comment by Andy Fingerhut [ 12/Feb/16 3:48 AM ]

Stephen, please see here http://dev.clojure.org/display/community/Developing+Patches for the commands used to create patches in the desired format.

Only a screener or Rich can say whether the patch is acceptable in the ways that matter for committing into Clojure.

Comment by Stephen Hopper [ 12/Feb/16 7:42 AM ]

I've corrected the format on my patch files and resubmitted them as one patch. Let me know if you see any issues with this. Also, it's worth pointing out that the first two patch files can be ignored / deleted.

Comment by Andy Fingerhut [ 14/Feb/16 1:30 PM ]

There are also instructions on that page for deleting old attachments, in the section titled "Removing patches", if you wished to do that.

A very minor comment, as the email address you use in your patches is completely up to you (as far as I know), but the one you have in your patch doesn't look like one that others could use to send you a message. If that was intentional on your part, no problem.

Comment by Stephen Hopper [ 14/Feb/16 1:48 PM ]

I've corrected the email address snafoo and removed the outdated patches. Thanks for walking me through this stuff, Andy.

Comment by Steve Miner [ 23/Feb/16 3:43 PM ]

The suggested fix is to make (hash -0.0) return the same as (hash 0.0). With the proposed patch, both will return 0. That happens to be the same as (hash 0). Not wrong at all, but maybe slightly less good than something else. Since you're making a change anyway, why not go the other way and use the -0.0 case as the common result?

I'm thinking that it would be a useful property if the hash of a long N is not equal to the hash of the corresponding (double N). In Clojure 1.8, zero is the only value I could quickly find where the long and the double equivalents have the same hash value.

The patch could be slightly tweaked to make (hash 0.0) and (hash -0.0)

return new Double(-0.0).hashCode()

The only change to the patch is to add the negative sign. The new hash result is -2147483648.

Admittedly this is an edge case, not a real performance issue. People probably don't mix longs and doubles in sets anyway. On the other hand, zeroes are kind of common. Since you're proposing a change, I thought it's worth considering a slight tweak.

Comment by Steve Miner [ 23/Feb/16 3:46 PM ]

Same for the float case, of course.

Comment by Alex Miller [ 24/Feb/16 1:36 PM ]

Looking at this agin, I'm ok with the approach and agree it's definitely a smaller change. Few additional changes needed in the patch and then I'll move it along:

  • Rather than `new Double(0.0).hashCode()` and `new Float(0.0).hashCode()`, move those values into `private static final int` constants and just return them.
  • In the comparison tests, throw -0.0M in there as well
  • Squash the patch into a single commit

Re Steve's suggestion, I do not think it's critical that long and double 0 hash differently and would prefer that double hashes match Java hashCode, so I would veto that suggestion.

Comment by Stephen Hopper [ 24/Feb/16 1:52 PM ]

Thank you, Alex. I'll make those updates.

Comment by Stephen Hopper [ 24/Feb/16 2:43 PM ]

Alex, I've attached the new patch file (CLJ-1860-negative-zero-hash-eq-fix.patch) to address the points from your previous post. Let me know if I missed anything.

Thanks!

Comment by Mike Anderson [ 25/Feb/16 7:44 PM ]

I may be late to the party since I have only just seen this, but I have a strong belief that 0.0 and -0.0 should be == but not =.

Reasons:

  • Anyone doing numerical comparison should use ==, so you want the result to be true
  • Anyone doing value comparison should use =, so you want the result to be false because these are different IEE784 double values. This includes set membership tests etc.

i.e. the Java code is doing it right, and we should be consistent with this.

Comment by Alex Miller [ 25/Feb/16 11:52 PM ]

I think there is consensus that == should be true, so we can set that aside and focus on =.

The reality is that there is no easy way to compare two collections with == (for example comparing [5.0 0.0 1.0] and [5.0 -0.0 1.0]). This is an actual use case that has been problematic for multiple people. While I grant there are use cases where 0.0 and -0.0 are usefully differentiable, I do not know of a real case in the community where this is the desired behavior, so I would rather err on the side of satisfying the intuition of the larger (and currently affected) population.

Also note that the Java code is doing it BOTH ways (primitive doubles are equal, boxed doubles are not), so I think that's a weak argument.

Comment by Alex Miller [ 26/Feb/16 8:44 AM ]

(after sleeping more on this...) It's possible that a better answer here is to expand what can be done with ==. I trust that when Rich looks at this ticket he will have his opinions which may or may not match up to mine and if so, we'll go in a different direction.

Comment by Andy Fingerhut [ 27/Feb/16 10:15 AM ]

Attachment clj-1860-make-equals-false-for-pos-neg-0.0-v1.patch dated Feb 27 2016 is a first cut at implementing a change where = returns false when comparing positive and negative 0.0, float or double.

As far as I can tell, there is no notion of positive and negative 0 for BigDecimal, so no change in behavior there.

Comment by Alex Miller [ 17/Nov/16 4:16 PM ]

Rich says: "0.0=-0.0, make hash the same"





[CLJ-2068] s/explain of evaluated predicate yields :s/unknown Created: 23/Nov/16  Updated: 10/Mar/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: Alex Miller
Resolution: Unresolved Votes: 1
Labels: spec

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

 Description   

Got:

(s/explain #{1 2 3} 4)
val: 4 fails predicate: :clojure.spec/unknown

(s/explain odd? 10)
val: 10 fails predicate: :clojure.spec/unknown

Expected to receive a description of the failing predicate as in:

(s/def ::s #{1 2 3})
(s/explain ::s 4)
;; val: 4 fails spec: :user/s predicate: #{1 3 2}

(s/def ::o odd?)
(s/explain ::o 10)
val: 10 fails spec: :user/o predicate: odd?

Cause: specize was falling through on these cases to Object and just returning unknown.

Proposed:
Special handling for 2 cases:
1. Sets - explictly catch IPersistentSet and use the set as the form.
2. Functions - demunge the function name and use the qualified function name symbol as the form. Add a special check for anonymous functions and revert to ::unknown for those (not much we can do with an eval'ed anonymous function).

Patch: clj-2068-2.patch



 Comments   
Comment by Alex Miller [ 13/Dec/16 6:52 PM ]

Simplified anon fn check and added a few basic tests.

Comment by Stuart Halloway [ 10/Mar/17 11:22 AM ]

Could the Specize protocol be extended to IFn, reducing the iffiness?

Comment by Alex Miller [ 10/Mar/17 11:39 AM ]

Yes, I think that would make sense.

Comment by Ghadi Shayban [ 10/Mar/17 12:57 PM ]

Extending Specize to IFn may incur the wrath of CLJ-1152





[CLJ-2123] Lighter-weight aliasing for keywords Created: 10/Mar/17  Updated: 10/Mar/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: 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-2065] reduce-kv fails on subvec Created: 20/Nov/16  Updated: 09/Mar/17

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

Type: Defect Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Approval: Triaged

 Description   

reduce-kv works as expected on vectors with the element index passed as the "key" argument. However, it fails with a subvec because clojure.lang.APersistentVector$SubVector does not implement IKVReduce.

(reduce-kv + 0 [1 2 3])
9

(reduce-kv + 0 (subvec [1 2 3] 1))
IllegalArgumentException No implementation of method: :kv-reduce of protocol: #'clojure.core.protocols/IKVReduce found for class: clojure.lang.APersistentVector$SubVector clojure.core/-cache-protocol-fn (core_deftype.clj:583)

One work around is to copy the subvec into a vector:

(reduce-kv + 0 (into [] (subvec [1 2 3] 1)))
6

Note however, the `vec` would not work here. Since Clojure 1.7, vec will return a subvec rather than copying.

(reduce-kv + 0 (vec (subvec [1 2 3] 1)))
IllegalArgumentException No implementation of method: :kv-reduce of protocol: #'clojure.core.protocols/IKVReduce found for class: clojure.lang.APersistentVector$SubVector clojure.core/-cache-protocol-fn (core_deftype.clj:583)



 Comments   
Comment by Steve Miner [ 20/Nov/16 12:53 PM ]

Here is my current work-around:

(extend-type clojure.lang.APersistentVector$SubVector
  clojure.core.protocols/IKVReduce
  (kv-reduce [subv f init]
    (transduce (map-indexed vector)
               (fn ([ret] ret) ([ret [k v]] (f ret k v)))
               init
               subv)))

In my tests it was usually faster to copy the subvec into a regular vector but I like the look of the transduce fix. It would probably be faster to add a native Java implementation in APersistentVector.java. I'm willing to do the work if the Clojure/core team wants a patch.

Comment by Steve Miner [ 09/Mar/17 1:38 PM ]

Revised work-around using more interop for better performance. Comparable to the speed of normal vector reduce-kv.

(when-not (satisfies?   clojure.core.protocols/IKVReduce (subvec [1] 0))
  (extend-type clojure.lang.APersistentVector$SubVector
    clojure.core.protocols/IKVReduce
    (kv-reduce
      [subv f init]
      (let [cnt (.count subv)]
        (loop [k 0 ret init]
          (if (< k cnt)
            (let [val (.nth subv k)
                  ret (f ret k val)]
              (if (reduced? ret)
                @ret
                (recur (inc k) ret)))
            ret))))))




[CLJ-2041] clojure.spec/keys requires input collections conform to clojure.core/map? Created: 11/Oct/16  Updated: 08/Mar/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: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: spec

Approval: Vetted

 Description   

I would like to use specs to validate Datomic entities. However, `s/keys` is too restrictive in that it requires input collections to conform to `clojure.core/map?` instead of some more primitive interface (for example clojure.lang.ILookup or clojure.lang.Associative).



 Comments   
Comment by Alex Miller [ 04/Nov/16 8:21 AM ]

s/keys uses IPersistentMap's Iterable support for iterating through all entries for validation. ILookup and Associative do not support iteration. So, that's why it is the way it is. But, understand the desire.

Comment by Alex Miller [ 04/Nov/16 8:34 AM ]

Datomic entities are seqable so maybe that's a potential path (would be slower for actual PHMs though).

Comment by Odin Standal [ 04/Nov/16 8:35 AM ]

Thanks for following up. So any ideas or guidance on how to use clojure.spec with Datomic entities?

Comment by Alex Miller [ 04/Nov/16 8:37 AM ]

For now, you could use into to pour an entity into a PHM before validating. I hesitate to suggest it, but that could even be in the spec with a leading conformer.

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

Moving this into 1.9 for the moment just so we don't lose it. Not sure whether we can or will actually do anything with this though.

Comment by Alex Miller [ 08/Mar/17 10:00 AM ]

This is related to CLJ-2080 as it's the same basic issue of what map-of and every-kv actually expect as valid inputs, which is: something that seqs to map entries. We can't really write a predicate for it without an interface "ISeqsToMapEntries" (intentionally bad name) to indicate this. java.util.Map, IPersistentMap, etc imply this, but an object can seq to map entries without meeting all the constraints of those much broader interfaces. ILookup does not and should not imply this.





[CLJ-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-1859] Update parameter name to reflect docstring Created: 30/Nov/15  Updated: 04/Mar/17

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

Type: Enhancement Priority: Trivial
Reporter: Matthew Boston Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: docstring

Attachments: Text File CLJ-1859-Update-parameter-name-to-reflect-docstring.patch    
Approval: Triaged

 Description   

The docstrings for `zero?`, `pos?`, and `neg?` reference `num` but the parameter is named `x`. This issue is to update the name of the parameter to `num` to reflect the docstring.



 Comments   
Comment by Alex Miller [ 30/Nov/15 1:14 PM ]

The inline fns should be updated too.

Comment by Matthew Boston [ 30/Nov/15 1:22 PM ]

Thanks, Alex. I was trying to follow the existing pattern that the inline functions have shorter parameter names. New patch attached.

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

CLJ-2121 should be closed as a duplicate of this, I assume?





[CLJ-2116] Support for selective conforming with clojure.spec Created: 22/Feb/17  Updated: 28/Feb/17

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

Type: Enhancement Priority: Major
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: spec
Environment:

[org.clojure/clojure "1.9.0-alpha14"]



 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.





[CLJ-2115] Support data conveying conform errors (alternative/complement to :clojure.spec/invalid) Created: 22/Feb/17  Updated: 22/Feb/17

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

Type: Enhancement 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-1496] Added a new arity to 'ex-info' that only accepts a message. Created: 08/Aug/14  Updated: 21/Feb/17

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

Type: Enhancement Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: try-catch
Environment:

java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)

Mac OSX 10.9.4


Attachments: File ex_info_arity.diff    
Patch: Code
Approval: Triaged

 Description   

We often use 'ex-info' to throw a custom exception.But ex-info at least accepts two arguments: a string message and a data map.
In most cases,but we don't need to throw a exception that taken a data map.
So i think we can add a new arity to ex-info:

(ex-info "the exception message")

That created a ExceptionInfo instance carries empty data.

I am not sure it's useful for other people,but it's really useful for our developers.

The patch is attached.



 Comments   
Comment by Alex Miller [ 10/Feb/17 8:46 AM ]

Why "(. clojure.lang.PersistentArrayMap EMPTY)" ? Why not just {}?

Comment by Leon Grapenthin [ 14/Feb/17 1:43 PM ]

I always thought the lack of a one-arity was intentional design to make users use the map argument. Why do you want to throw ExceptionInfos with no data?

Comment by dennis zhuang [ 14/Feb/17 9:53 PM ]

@Alex I forgot why i used EMPTY map here, maybe influenced by the code https://github.com/clojure/clojure/blob/7aad2f7dbb3a66019e5cef3726d52d721e9c60df/src/clj/clojure/core.clj#L4336

@Leon For example, throw an exception when arguments error:

(when-not (integer? c)
(throw (ex-info "Expect number for c."))

We don't need data here.

Comment by Nicola Mometto [ 15/Feb/17 4:27 AM ]

Then why not just throw an `(Exception. "expect number for c")`? I don't see the added value in throwing exinfos w/o data vs just throwing an Exception

Comment by dennis zhuang [ 21/Feb/17 9:13 PM ]

Indeed, it was just a technical decision that we chose to use ex-info for throwing exceptions.





[CLJ-2103] s/coll-of and s/every take unnecessary long to generate if :into not provided Created: 28/Jan/17  Updated: 17/Feb/17

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

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

alpha14



 Description   
(s/def ::bar (s/coll-of number? :kind vector?))

(defn foo [a]
  (reduce + 0 a))

(s/fdef foo
  :args (s/cat :a ::bar)
  :ret number?)

(first (stest/check `foo))

Doesn't terminate in a reasonable amount of time.
One observes that changing :gen-max doesn't affect computation time.

One observes that it can be fixed by adding :into [] to the s/coll-of spec.

The reason is this: If :into is not provided, s/every and s/coll-of has to generate a vector (via gen of vector?) 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 vector? generates quite large vectors at a certain point which significantly slows down the generation.

The responsible code is in gen* of every-impl

(gen/bind
 (cond
  gen-into (gen/return (empty gen-into))
  kind (gen/fmap #(if (empty? %) % (empty %))
                 (gensub kind overrides path rmap form))  ;; creating and mapping gen of :kind
 :else (gen/return []))
 (fn [init]
    ....


 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-1138] data-reader returning nil causes exception Created: 22/Dec/12  Updated: 15/Feb/17

Status: Open
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: Defect Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: reader
Environment:

clojure 1.5 beta2, Mac OS X 10.8.2, java version "1.6.0_37"


Attachments: Text File 0001-CLJ-1139-allow-nil-in-data-reader.patch     Text File clj-1139-2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

If a data-reader returns nil, the reader throws java.lang.RuntimeException: No dispatch macro... The error message implies that there is no dispatch macro for whatever the first character of the tag happens to be.

Here's a simple example:

user=> (binding [*data-readers* {'f/ignore (constantly nil)}] 
         (read-string "#f/ignore 42 10"))
RuntimeException No dispatch macro for: f  clojure.lang.Util.runtimeException (Util.java:219)

The original reader code did not distinguish between the absence of a data-reader and a returned value of nil from the appropriate data-reader. It therefore got confused and tried to find a dispatch macro, sending it further down the incorrect code path, ultimately yielding a misleading error message.

The original documentation did not distinguish nil as an illegal value. Clearly this bug was an oversight in the original data-reader code, not an intentional feature.

The patch uses a sentinel value to distinguish the missing data-reader case from the nil returned value case.

Patch: clj-1139-2.patch



 Comments   
Comment by Steve Miner [ 22/Dec/12 9:43 AM ]

clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch allows a data-reader to return nil instead of throwing. Does sanity check that possible tag or record isJavaIdentifierStart(). Gives better error message for special characters that might actually be dispatch macros (rather than assuming it's a tagged literal).

Comment by Steve Miner [ 22/Dec/12 10:06 AM ]

clj-1138-data-reader-return-nil-for-no-op.patch allows a data-reader returning nil to be treated as a no-op by the reader (like #_). nil is not normally a useful value (actually it causes an exception in Clojure 1.4 through 1.5 beta2) for a data-reader to return. With this patch, one could get something like a conditional feature reader using data-readers.

Comment by Steve Miner [ 22/Dec/12 10:26 AM ]

clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch is the first patch to consider. It merely allows nil as a value from a data-reader and returns nil as the final value. I think it does what was originally intended for dispatch macros, and gives a better error message in many cases (mostly typos).

The second patch, clj-1138-data-reader-return-nil-for-no-op.patch, depends on the other being applied first. It takes an extra step to treat a nil value returned from a data-reader as a no-op for the reader (like #_).

Comment by Steve Miner [ 23/Dec/12 11:52 AM ]

It turns out that you can work around the original problem by having your data-reader return '(quote nil) instead of plain nil. That expression conveniently evaluates to nil so you can get a nil if necessary. This also works after applying the patches so there's still a way to return nil if you really want it.

(binding [*data-readers* {'x/nil (constantly '(quote nil))}] (read-string "#x/nil 42"))
;=> (quote nil)

Comment by Andy Fingerhut [ 07/Feb/13 9:20 AM ]

Patch clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch dated Dec 22 2012 still applies cleanly to latest master if you use the following command:

% git am --keep-cr -s --ignore-whitespace < clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch

Without the --ignore-whitespace option, the patch fails only because some whitespace was changed in Clojure master recently.

Comment by Andy Fingerhut [ 13/Feb/13 11:24 AM ]

OK, now with latest master (1.5.0-RC15 at this time), patch clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch no longer applies cleanly, not even using --ignore-whitespace in the 'git am' command given above. Steve, if you could see what needs to be updated, that would be great. Using the patch command as suggested in the "Updating stale patches" section of http://dev.clojure.org/display/design/JIRA+workflow wasn't enough, so it should probably be carefully examined by hand to see what needs updating.

Comment by Steve Miner [ 14/Feb/13 12:21 PM ]

I removed my patches. Things have changes recently with the LispReader and new EdnReader.

Comment by Alex Miller [ 15/Feb/17 9:18 AM ]

Fixed whitespace warning and updated patch so it applies, no semantic changes, attribution retained in clj-1139-2.patch.

Comment by Alex Miller [ 15/Feb/17 9:27 AM ]

Ticket needs better description of problem and approach taken in the patch.

Comment by Steve Miner [ 15/Feb/17 10:09 AM ]

If the problem isn't clear, I would ask why would a nil return value be treated specially for a data-reader? And if it is considered illegal by design, does this error message enlighten the user?

I could not find any documented restriction at the time the bug was filed and I still can't find any today. So it seems like a simple bug to me. The data-reader should be allowed to return nil, and the Clojure reader should process the nil as usual. My work-around was to return (quote nil) which gave the intended behavior without triggering the bug.

Comment by Alex Miller [ 15/Feb/17 10:55 AM ]

Would appreciate more updates to the description. My question would be whether invoking a data reader function should ever return nil. Is there a good use case to need this? It seems you are reading the description of a non-nil tagged value with the reader and thus getting back nil is confusing. That's not possibly round-trippable and thus seems asymmetric.

Comment by Steve Miner [ 15/Feb/17 1:34 PM ]

Nulla poena sine lege or basically unless you say it's illegal, I should be able to do it. My trivial example is #C NULL which seems like an obvious nil to me.

Looking back on this issue, I can see that most people think of tagged literals as a way of encoding foreign values in Clojure literals. If you only care about an extensible data notation, who needs another way of writing nil? That's a fair question.

I wanted to use data-readers as somewhat circumscribed reader macros (as used in Common Lisp). I discovered this bug while I was doing something platform specific (long before reader conditionals were implemented). In my situation, it was convenient to return nil on "other" platforms.

Many usages of data-readers are not bijective. For example, #infix (3 + 4) interpreted as constant 7 is likewise not round-trippable. Unless you're Dan Friedman or Wil Byrd, round-tripping is a tough requirement.

I will try to update my description with a bit more context, but I don't want to distract anyone from the obvious bug (and bad error message) with my unorthodox usage.

Comment by Steve Miner [ 15/Feb/17 1:45 PM ]

By the way, this bug is CLJ-1138, but the proposed patch says "1139" which might confuse some busy reviewers.

Comment by Steve Miner [ 15/Feb/17 2:13 PM ]

I tested the patch and it worked well for me with the current master. I would suggest adding another test to confirm that the edn/read-string works correctly as well. Here's what I used. This also tests that overriding the default readers works. Please feel free to take the test if you want it.

(deftest clj-1138-uuid-override
  (is (nil? (binding [*data-readers* {'uuid (constantly nil)}]
              (read-string "#uuid \"550e8400-e29b-41d4-a716-446655440000\""))))
  (is (nil? (edn/read-string {:readers {'uuid (constantly nil)}}
                             "#uuid \"550e8400-e29b-41d4-a716-446655440000\""))))




[CLJ-2112] Add specs for spec forms Created: 15/Feb/17  Updated: 15/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: 11
Labels: spec

Attachments: Text File spec-forms.patch    
Patch: Code
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-2040] Allow runtime modification of REPL exception handling Created: 11/Oct/16  Updated: 15/Feb/17

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

Type: Enhancement Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: repl

Attachments: Text File CLJ-2040-dynamic-repl-exceptions.patch    
Patch: Code
Approval: Triaged

 Description   

Problem Statement

Clojure's REPL is capable of paramterizing almost every aspect of its functionality, including how uncaught exceptions are printed. In the current implementation, these customization hooks are passed in as arguments and closed over, meaning that they cannot be changed once the REPL is started.

Many development tools want to override how the REPL handles uncaught errors. Examples of useful customizations include (but are not limited to):

  • Formatted exception messages (including whitespace and ANSI coloring)
  • Alternative representations for certain types of exceptions (e.g, Spec errors)
  • Dropping into a graphical interaction mode to better inspect ex-data.

Currently, this type of customization must be applied before a REPL is started, meaning that changing how a REPL displays errors requires support from (or plugins to) a third-party tool such as Boot or Leiningen.

Alternatives


1. Take no action.

Third-party tool support is required to create customized exception handling in the REPL. Tools have different techniques for doing this:

  • nREPL can intercept the exception on the wire and passes it through middleware
  • Leiningen plugins alter the root binding of clojure.main/repl-caught.
  • Boot allows users to build a task to invoke clojure.main/repl with the desired arguments.

Users will continue to select one of these according to their tooling preferences.

Benefits:
1. No effort or changes to the existing code.

Tradeoffs:
1. Tools will continue to implement their own diverse, sometimes hacky techniques for printing custom exceptions.
2. Any library intended to provide alternative exception handling will be tied to a specific launcher tool.

2. Make the REPL exception handler dynamically rebindable

If the REPL exception handler were a dynamic, thread-local var, users and libraries could change the behavior of the currently running REPL.

Benefits:
1. Users and libraries can freely override how exceptions are printed, regardless of how Clojure was launched.
2. Fully backwards compatible with existing tools.

Tradeoffs:
1. It will be possible for library authors to provide "bad" or poorly reasoned error printers. This is still possible with launch tools, but the barrier of entry is even lower with libraries.

The attached patch implements this option.

3. Encourage users to start new REPLs instead

In many Clojure environments, it's possible to explicitly launch a REPL from within another REPL. This sub-REPL could have the desired :caught hook.

Benefits:
1. No effort or changes to the existing code.
2. "Functionally pure", and in alignment with the evident design of the current REPL.

Tradeoffs:
1. There is a non-trivial subset of Clojure developers who do not know exactly how REPLs work. They are likely to be confused or subject to increased cognitive load. Insofar as this set of beginner/intermediate developers are precisely who enhanced error messages are meant to help in the first place, this solution is counterproductive.
2. For better or for worse, many existing and widely used tools do not support this. This does not work at all in nREPL, for example. However, even the simplest command-line REPLs behavior would change for the worse; sending a EOF (accidentally or otherwise) would always kill the sub-REPL with no feedback as to what just happened.



 Comments   
Comment by Alex Miller [ 15/Feb/17 9:39 AM ]

On the repl-caught var, it would be good to mention the signature of the handler, namely that it takes an exception, is expected to print or otherwise handle the exception, and that its return will be ignored.

Rather than the changes in repl, what if you added a dynamic-repl-caught and change that to be the default caught handler in repl? Or even just changed repl-caught itself.





[CLJ-2111] s/every :kind does not work as intended Created: 14/Feb/17  Updated: 14/Feb/17

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

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

Approval: Vetted

 Description   
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)

Expected: true



 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-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-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: 1
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-1776] Test that collections are valid statements Created: 08/Jul/15  Updated: 09/Feb/17

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

Type: Enhancement Priority: Minor
Reporter: Michael Blume Assignee: Michael Blume
Resolution: Unresolved Votes: 0
Labels: compiler, test

Attachments: Text File clj-1776-v1.patch     Text File CLJ-1776-v2.patch    
Patch: Code and Test

 Description   

It's possible to break the compiler such that vectors are emitted incorrectly when they're in statement position (I accidentally did this). This doesn't break any part of the Clojure test suite, but does break valid Clojure code (for me it hit taoensso's encore). Add tests to the test suite so defects of this kind are caught.



 Comments   
Comment by Daniel Compton [ 09/Feb/17 8:34 PM ]

Is there a compiler change required as well here?

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

I think he is saying that the compiler is fine but that if it weren't, no test would tell you that.





[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-2056] Efficient shortcut for (first (filter pred coll)) idiom Created: 11/Nov/16  Updated: 30/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: 36
Labels: None

Attachments: Text File clj-2056-clojure-core-seek-2.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

It's a common task to look up for an item in a collection based on predicate. Currently Clojure has no direct support for that in clojure.core. Instead, our options are:

1. (first (filter pred coll)) will create intermediate lazy sequence and might evaluate pred up to 31 extra times in case of chunked sequence

2. (some #(when (pred %) %) coll) will short-circuit on first match, but won't catch false value in something like (some #(when false? %) [true false true])

Additionally, both of these workarounds a) obscure the purpose of the code, and b) do not handle custom not-found values.

Attached is a patch that makes use of efficiency of reduce-able collections, handles edge cases like looking for false? or nil?, and supports optional not-found value.

Examples:

(seek odd? (range)) => 1
(seek pos? [-1 1]) => 1
(seek pos? [-1 -2] ::not-found) => ::not-found
(seek nil? [1 2 nil 3] ::not-found) => nil

Patch: clj-2056-clojure-core-seek-2.patch

Prescreening notes: I think the general approach is good. Is it necessary to support nil? and false? preds? Or would a transduce formulation like the one in comments be sufficient.



 Comments   
Comment by Alex Miller [ 11/Nov/16 8:54 AM ]

Just as an interesting aside, the new halt-when transducer could actually be used to create something like this too (if you set aside the desire to support nil? and false? preds).

(transduce (comp (filter pred) (halt-when any?)) identity nil coll)

Patch has some trailing whitespace in the test code - could you clean that up?

Comment by Nikita Prokopov [ 12/Nov/16 3:46 AM ]

Attaching patch with trailing whitespace cleaned

Comment by Nikita Prokopov [ 12/Nov/16 3:46 AM ]

Thanks Alex! Attached new patch with whitespace cleaned

Comment by Moritz Heidkamp [ 30/Jan/17 12:34 PM ]

I had a similar train of thought today and arrived at the idea of adding transducer support to first, e.g. (first (filter pred) coll). This could potentially be as efficient as the special-purpose seek function proposed above but would of course not support the not-found value. However, it seems like a natural extension to first and could be useful for other purposes, too.





[CLJ-2104] Typo in pprint docstring Created: 29/Jan/17  Updated: 29/Jan/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"






[CLJ-2025] When a generator fails to gen, state which spec/pred failed Created: 21/Sep/16  Updated: 29/Jan/17

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

Type: Enhancement Priority: Minor
Reporter: David Collie Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec

Attachments: Text File better-such-that-info.patch    
Approval: Triaged

 Description   

Given a generator using such-that that fails to find a value, the error does not give enough information to determine which spec or predicate was at fault:

(require '[clojure.spec :as s])
(s/exercise (s/and string? #{"hi"}))
ExceptionInfo Couldn't satisfy such-that predicate after 100 tries.  clojure.core/ex-info (core.clj:4725)

Another special case of this is when providing a custom generator that produces a valid that doesn't satisfy the spec (Clojure adds this filter internally):

(require '[clojure.spec :as s])
(s/exercise (s/with-gen int? #(s/gen #{:a})))

Proposal: Indicate in the error which spec failed to generate and possibly the path in the overall spec if feasible.

(Note: original description moved to comment)



 Comments   
Comment by Alex Miller [ 22/Sep/16 3:54 PM ]

[Original description from ticket:]

I created a generator that did not conform to the spec (doh!). The generator contained the such-that predicate. When I tried creating a sample from the generator I got this error:

ExceptionInfo Couldn't satisfy such-that predicate after 100 tries.  clojure.core/ex-info (core.clj:4725)

I assumed that it referred to my custom generator but that was a red herring because in fact spec must be using such-that to ensure that the generated value conforms to the spec, and it was this such-that that generated the failure, not the one in my custom generator.

Code (with the problem corrected but showing the such-that in my generator:

(defn mod11-checkdigit
  "Calculate the checkdigit see http://freagra.com/imthealth/mitNNC.html"
  [n]
  (let [x (->> (map #(Integer/parseInt (str %)) (take 9 n))
               (map * (range 10 1 -1))
               (reduce +))
        y (mod x 11)
        c (- 11 y)]
    (cond (== 10 c) nil
          (== 11 c) 0
          :else c)))

(def nhs-number-gen
  "Generates a valid NHS number"
  (gen/fmap #(str (+ (* 10 %) (mod11-checkdigit (str %))))
            (gen/such-that #(mod11-checkdigit (str %))
                           (gen/choose 100000000 999999999))))

(defn nhs-number?
  "Returns true if passed a valid nhs number else returns false"
  [n]
  (and (string? n) (= 10 (count n)) (= (str (mod11-checkdigit n)) (str (last n)))))

(s/def ::nhs-number (s/with-gen nhs-number?
                                (fn [] nhs-number-gen)))

It would be nicer if the error thrown due to the generated value being non-conformant with the spec stated this.

Comment by Alex Miller [ 22/Sep/16 4:07 PM ]

I'm not sure that this is possible right now based on what we give to and get back from test.check.

Comment by Griffin Smith [ 29/Jan/17 6:37 PM ]

It looks like test.check is being updated to support error customization: https://github.com/clojure/test.check/commit/5aea0e275257680b672309b1e940be6dae92c17d . I've got a patch which updates clojure.spec to use it, though obviously it doesn't work since the linked commit hasn't made it into a released version of test.check yet.

Comment by Griffin Smith [ 29/Jan/17 6:40 PM ]

See also http://dev.clojure.org/jira/browse/TCHECK-107 I suppose

Comment by Griffin Smith [ 29/Jan/17 6:51 PM ]

Attached the patch for future reference (`better-such-that-info.patch`).





[CLJ-2021] case where spec/conform -> spec/unform -> spec/conform gives invalid result Created: 12/Sep/16  Updated: 28/Jan/17

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

Type: Defect Priority: Major
Reporter: Jeroen van Dijk Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: spec
Environment:

clojure 1.9, mac osx, java 1.8



 Description   

The example belows shows a case where a conform-ed form, does not conform any after an unform. It would be my expectation that you can repeat conform -> unform -> conform endlessly and get the same result.

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

(s/def ::defn-macro (s/cat :type #

Unknown macro: {'defn}
:definition :clojure.core.specs/defn-args))

(let [form '(defn foo "bar" ([a & b] a a c) ([a b] a))]

(-> form
(->> (s/conform ::defn-macro))) ;;=> {:type defn, :definition {:name foo, :docstring "bar", :bs [:arity-n {:bodies [{:args {:args [[:sym a]], :varargs {:amp &, :form [:sym b]}}, :body [a a c]} {:args {:args [[:sym a] [:sym b]]}, :body [a]}]}]}}

;; Unforming returns the function definition, but with the args in a list instead of a vector:
(->> form
(s/conform ::defn-macro)
(s/unform ::defn-macro)) ;;=> (defn foo "bar" ((a (& b)) a a c) ((a b) a)))

;; Conforming after unforming doesn't work anymore
(->> form
(s/conform ::defn-macro)
(s/unform ::defn-macro)
(s/conform ::defn-macro)) ;;=> :clojure.spec/invalid

)



 Comments   
Comment by Jeroen van Dijk [ 12/Sep/16 8:22 AM ]

This gist shows the above code with better formatting https://gist.github.com/jeroenvandijk/28c6cdd867dbc9889565dca92673a531

Comment by Leon Grapenthin [ 28/Jan/17 4:49 PM ]

This can quickly be traced down to :clojure.core.specs/arg-list which is speced as a (s/and <regex> vector?). When unforming, it doesn't create a vector.

Thinking about it, a vcat would be nice for this and similar cases.





[CLJ-2013] Alternative s/cat options not error-reported Created: 24/Aug/16  Updated: 28/Jan/17

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

Type: Enhancement Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, spec
Environment:

alpha14


Attachments: Text File CLJ-2013.patch    
Approval: Triaged

 Description   

This problem was detected in context of this discussion https://groups.google.com/d/msg/clojure/mIlKaOiujlo/tF71zZ2BCwAJ

A minimal version of how specs error reporting failed the users intuition there looks like this:

He used an invalid ns form

(ns foo (require [clojure.spec :as s])) ; should be :require

The error reported by spec:

In: [1] val: ((require [clojure.spec :as s])) fails at: [:args] predicate: (cat :docstring (? string?) :attr-map (? map?) :clauses :clojure.core.specs/ns-clauses),  Extra input
:clojure.spec/args  (foo (require [clojure.spec :as s]))
  clojure.core/ex-info (core.clj:4725)

While the error is technically true, it doesn't show the user /how/ each of the alternative options of the reported s/cat failed.

To get a better understanding why the users data is not correct, he should know precisely what spec tried and how it failed.

A good example of how this works is s/alt, where all failing alternatives are always reported to the user.

The problem has been investigated, first experimentally, then in specs code. Finally, a patch that brings error reporting like s/alts comes attached.

It has been observed that specs error reporting behavior for cat with optional branches is the following:

1. If the cat failed after one or many optional branches, the entire cat is reported as failing
2. If the cat failed after one or many optional branches /and/ a subsequent required branch, only the subsequent required branch is reported with no remarks to the alternative optional branches.

Rule 1 explains the ns example.
Rule 2 can fail the users intuition significantly worse:

(s/explain (s/cat :maybe-num (s/? number?)
                  :keyword keyword?)
           ["3"])

gives

In: [0] val: "3" fails at: [:keyword] predicate: keyword?

The report clearly doesn't address the users intent of putting in a number. Instead he is made to believe that he should have entered a keyword.

Solution:

A simple patch has been programmed that changes op-explain to have the following behaviour:

  • All alternatives that have been tried in a s/cat are reported individually.

It improves the reported errors significantly because it makes clearly transparent how the users data failed the validation.

(ns foo (require [clojure.spec :as s])) ; should be :require

now gives

ExceptionInfo Call to clojure.core/ns did not conform to spec:
In: [1] val: (require [clojure.spec :as s]) fails at: [:args :docstring] predicate: string?
In: [1] val: (require [clojure.spec :as s]) fails at: [:args :attr-map] predicate: map?
In: [1 0] val: require fails spec: :clojure.core.specs/ns-refer-clojure at: [:args :clauses :refer-clojure :clause] predicate: #{:refer-clojure}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-require at: [:args :clauses :require :clause] predicate: #{:require}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-import at: [:args :clauses :import :clause] predicate: #{:import}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-use at: [:args :clauses :use :clause] predicate: #{:use}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-refer at: [:args :clauses :refer :clause] predicate: #{:refer}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-load at: [:args :clauses :load :clause] predicate: #{:load}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-gen-class at: [:args :clauses :gen-class :clause] predicate: #{:gen-class}
:clojure.spec/args  (foo (require [clojure.spec :as s]))
  clojure.core/ex-info (core.clj:4725)

It would be even better if explain-data sorted ::s/problems by length of their :path which would push the first two unintended options to the end.

(s/explain (s/cat :maybe-num (s/? number?)
                  :keyword keyword?)
           ["3"])

now gives

In: [0] val: "3" fails at: [:maybe-num] predicate: number?
In: [0] val: "3" fails at: [:keyword] predicate: keyword?

While examples can be made up where this reporting produces more noise (like defn) I believe its the right tradeoff for aforementioned reasons and:

  • We programmers always ask our users for the most specific information when something went wrong - It is correct to apply the same to specs error reporting
  • Custom error reporters (s/explain-out) get more data to generate narrow reports matching the users intent even better


 Comments   
Comment by Nuttanart Pornprasitsakul [ 12/Oct/16 8:43 AM ]

I'll put a somewhat different issue here because I think it has the same root cause.

It should specifically say :ret of fspec is missing but it says failing at :args.

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

(defn x [f] (f 1))

(s/fdef x
  :args (s/cat :f (s/fspec :args (s/cat :i int?))))

(st/instrument `x)

(x (fn [a] a))
Exception in thread "main" clojure.lang.ExceptionInfo: Call to #'user/x did not conform to spec:
In: [0] val: (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"]) fails at: [:args] predicate: (cat :f (fspec :args (cat :i int?))),  Extra input
:clojure.spec/args  (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"])
:clojure.spec/failure  :instrument
:clojure.spec.test/caller  {:file "debug.clj", :line 16, :var-scope user/eval20}
 {:clojure.spec/problems [{:path [:args], :reason "Extra input", :pred (cat :f (fspec :args (cat :i int?))), :val (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"]), :via [], :in [0]}], :clojure.spec/args (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"]), :clojure.spec/failure :instrument,
...




[CLJ-304] clojure.repl/source does not work with deftype Created: 20/Apr/10  Updated: 19/Jan/17

Status: In Progress
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 21
Labels: repl

Approval: Triaged

 Description   

clojure.repl/source does not work on a deftype

user> (deftype Foo [a b])
user.Foo
user> (source Foo)
Source not found

Cause: deftype creates a class but not a var so no file/line info is attached anywhere.

Approach:

Patch:

Screened by:



 Comments   
Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

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

Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

chouser@n01se.net said: That's a great question. get-source just needs a file name and line number.

If IMeta were a protocol, it could be extended to Class. That implementation could look for a "well-known" static field, perhaps? __clojure_meta or something? Then deftype would just have to populate that field, and get-source would be all set.

Does that plan have any merit? Is there a better place to store a file name and line number?

Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

stu said: Seems like a reasonable idea, but this is going to get back-burnered for now, unless there is a dire use case we have missed.

Comment by Gary Trakhman [ 19/Feb/14 10:31 AM ]

I could use this for cider's file/line jump-around mechanism as well.

With records, I can work around it by deriving and finding the corresponding constructor var, but it's a bit nasty.

Comment by Bozhidar Batsov [ 03/Mar/14 6:37 AM ]

I'd also love to see this fixed.

Comment by Andy Fingerhut [ 03/Mar/14 8:33 AM ]

Bozhidar, voting on a ticket (clicking the Vote link in the right of the page when viewing the ticket) can help push it upwards on listings of tickets by # of votes.

Comment by Bozhidar Batsov [ 19/Sep/14 1:17 PM ]

Andy, thanks for the pointer. They should have made this button much bigger, I hadn't noticed it all until now.

Comment by Alex Miller [ 26/Jan/16 4:12 PM ]

If someone did some work on this (like a patch), I would push it harder.

Comment by Baptiste Dupuch [ 19/Jan/17 2:45 PM ]

it's "working" for me using this =>

(source source-training.source/->Foo)




[CLJ-1771] Support for multiple key(s)-value pairs in assoc-in Created: 29/Jun/15  Updated: 14/Jan/17

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

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

All


Attachments: Text File clj-1771.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

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

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

Patch: clj-1771.patch

Prescreened by: Alex Miller



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

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

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

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

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

Do you need the "if kvs" check?

Should have tests.

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

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

The patch now includes tests.

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





[CLJ-2089] Sorted colls with default comparator don't check that first element is Comparable Created: 19/Dec/16  Updated: 13/Jan/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: Vetted

 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






[CLJ-1368] Document usage for case with non-readable constants Created: 02/Mar/14  Updated: 13/Jan/17

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

Type: Enhancement Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docs, interop


 Description   

Problem

It is pretty obscure how to get constant-time dispatch for e.g. Enums, even if user knows about case.

Proposal

The possibility to dispatch to arbitrary constants with case, by wrapper macro, should be documented.

Wording

  • Should it warn against doing that with unstable values?
  • Should it mention anything else than java Enums?

Case Techniques

Case is documented for accepting all readable forms as test-constants. However, it can also be made to use any compile-time-known constants as test-constants, by wrapping it in another macro.

Sometimes this is appropriate, e.g. when dispatching on a java Enum.
Other times, less so, e.g. when dispatching on objects whose hash changes when the vm is restarted (breaks AOT).

Implications

This technique is an application of a more general technique: Passing non-literals to a macro from another macro.
Are there other macros that have use cases like this?

References

https://groups.google.com/d/topic/clojure/3yGjDO2YnjQ/discussion



 Comments   
Comment by Herwig Hochleitner [ 02/Mar/14 11:25 AM ]

This is a duplicate of http://dev.clojure.org/jira/browse/CLJ-1367

Actually, it's an alternate solution

Comment by Petr Gladkikh [ 13/Jan/17 5:58 AM ]

Probably this ticket and CLJ-1367 linger for so long because there's already 'condp' that can be used as follows:

(condp = test-value
  JavaClass/CONST1 result1
  JavaClass/CONST2 result2)

This is sequential and slower but is about as concise as plain case.

However if this is the form to be used instead of plain 'case' this should be suggested by documentation.





[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: 0
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-2074] ::keys spec conflicts with destructuring spec Created: 02/Dec/16  Updated: 11/Jan/17

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

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

Attachments: File close-destructuring-keys-specs.diff    
Patch: Code

 Description   

As a consequence of the destructuring specs being implemented in terms of `s/keys`, defining a spec for `::keys` or `::strs` is problematic at the moment, because it will conflict with trying to use `::keys` for destructuring:

user=> (require '[clojure.spec :as s])
nil
user=> (s/def ::keys nil?)
:user/keys
user=> (let [{::keys [a]} {::a 1}] a)
ExceptionInfo Call to clojure.core/let did not conform to spec:
In: [0 0] val: #:user{:keys [a]} fails spec: :clojure.core.specs/local-name at: [:args :bindings :binding :sym] predicate: simple-symbol?
In: [0 0 0] val: ([:user/keys [a]]) fails spec: :clojure.core.specs/seq-binding-form at: [:args :bindings :binding :seq] predicate: (cat :elems (* :clojure.core.specs/binding-form) :rest (? (cat :amp #{(quote &)} :form :clojure.core.specs/binding-form)) :as (? (cat :as #{:as} :sym :clojure.core.specs/local-name))),  Extra input
In: [0 0 :user/keys] val: [a] fails spec: :user/keys at: [:args :bindings :binding :map :user/keys] predicate: nil?
:clojure.spec/args  ([#:user{:keys [a]} #:user{:a 1}] a)
  clojure.core/ex-info (core.clj:4725)

This feels like an implementation detail leak.



 Comments   
Comment by Brandon Bloom [ 10/Jan/17 5:36 PM ]

I also just ran in to this problem. Just wanted to say that I'd like to see a fix, but I'm not quite sure about the proposed solution. Or, at least, the name "closed?" seems to imply a non-extensible map, when in reality the flag more or less means "not a map that participates in the global keys system", for which I do not have a better name suggestion.

Comment by Alex Miller [ 11/Jan/17 8:35 AM ]

The proposed patch is a non-starter. I have some ideas on how to address this, but just haven't gotten around to working on it yet.

Comment by Alex Miller [ 11/Jan/17 8:37 AM ]

Removed proposal and patch from the ticket as we will not be going this direction. Captured here for reference:

"The attached patch implements a proposed solution to this issue, by adding a `:closed?` option to `s/keys` and using it for the destructuring spec. If `s/keys` is used with `:closed?` set to true, `conform` will only validate declared specs as opposed to the default behaviour of `s/keys` of validating all namespaced keywords with existing specs.

After this patch, the above example runs fine and usages of `s/keys` without `:closed?` set to true will validate against `::keys` as per current behaviour.

Patch: close-destructuring-keys-specs.diff"





[CLJ-1865] Direct linking doesn't work on recursive calls Created: 08/Dec/15  Updated: 06/Jan/17

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, directlinking


 Description   

It looks like self-recursive calls aren't optimized by direct linking, but if we redefine the same function twice, the Compiler is tricked into thinking that the call is not recursive and (rightfully) optimizes it into an invokeStatic.

I haven't investigated the cause but I suspect (and I might be wrong) it has to do with :arglist metadata potentially having different values when the Var is undefined vs when it's already bound.

[~]> cat test.clj
(ns test)

(defn a [x]
  (a x))
[~]> clj
Clojure 1.8.0-master-SNAPSHOT
user=> (compile 'test)
test
user=> ^D
[~]> cd classes
[~/classes]> javap -c test\$a
Compiled from "test.clj"
public final class test$a extends clojure.lang.AFunction {
  public static final clojure.lang.Var const__0;

  public static {};
    Code:
       0: ldc           #11                 // String test
       2: ldc           #13                 // String a
       4: invokestatic  #19                 // Method clojure/lang/RT.var:(Ljava/lang/String;Ljava/lang/String;)Lclojure/lang/Var;
       7: checkcast     #21                 // class clojure/lang/Var
      10: putstatic     #23                 // Field const__0:Lclojure/lang/Var;
      13: return

  public test$a();
    Code:
       0: aload_0
       1: invokespecial #26                 // Method clojure/lang/AFunction."<init>":()V
       4: return

  public static java.lang.Object invokeStatic(java.lang.Object);
    Code:
       0: getstatic     #23                 // Field const__0:Lclojure/lang/Var;
       3: invokevirtual #32                 // Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
       6: checkcast     #34                 // class clojure/lang/IFn
       9: aload_0
      10: aconst_null
      11: astore_0
      12: invokeinterface #37,  2           // InterfaceMethod clojure/lang/IFn.invoke:(Ljava/lang/Object;)Ljava/lang/Object;
      17: areturn

  public java.lang.Object invoke(java.lang.Object);
    Code:
       0: aload_1
       1: aconst_null
       2: astore_1
       3: invokestatic  #41                 // Method invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
       6: areturn
}

Redefining the same function twice makes it work.

[~]> cat test.clj
(ns test)

(defn a [x]
  (a x))

(defn a [x]
  (a x))
[~]> clj
Clojure 1.8.0-master-SNAPSHOT
user=> (compile 'test)
test
user=> ^D
[~]> cd classes
[~/classes]> javap -c test\$a
Compiled from "test.clj"
public final class test$a extends clojure.lang.AFunction {
  public static final clojure.lang.Var const__0;

  public static {};
    Code:
       0: ldc           #11                 // String test
       2: ldc           #13                 // String a
       4: invokestatic  #19                 // Method clojure/lang/RT.var:(Ljava/lang/String;Ljava/lang/String;)Lclojure/lang/Var;
       7: checkcast     #21                 // class clojure/lang/Var
      10: putstatic     #23                 // Field const__0:Lclojure/lang/Var;
      13: return

  public test$a();
    Code:
       0: aload_0
       1: invokespecial #26                 // Method clojure/lang/AFunction."<init>":()V
       4: return

  public static java.lang.Object invokeStatic(java.lang.Object);
    Code:
       0: aload_0
       1: aconst_null
       2: astore_0
       3: invokestatic  #30                 // Method invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
       6: areturn

  public java.lang.Object invoke(java.lang.Object);
    Code:
       0: aload_1
       1: aconst_null
       2: astore_1
       3: invokestatic  #30                 // Method invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
       6: areturn
}


 Comments   
Comment by Nicola Mometto [ 06/Jan/17 10:12 AM ]

I just took a quick look at this, not an easy one to fix as handling recursive calls would likely require 2 passes over the whole defn AST, one to determine whether the defn is direct-linkable and the other one to build the AST using StaticInvokeExpr rather than InvokeExpr (using a stub Method using the analysis info rather than reflecting)

Comment by Alex Miller [ 06/Jan/17 2:03 PM ]

Yeah, Rich is aware of this and it's not done yet due to the issues you mentioned. It's hard!





[CLJ-1556] Add instance check functions to defrecord/deftype Created: 09/Oct/14  Updated: 06/Jan/17

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: defrecord, deftype

Attachments: Text File 0001-CLJ-1556-Generate-type-functions-with-instance-check.patch    
Patch: Code

 Description   

It is often necessarty to test for instance? on deftypes/defrecords, this patch makes the two macros automatically generate a type? function implemented as (fn [x] (instance? type x)), to complement ->type and map->type
Example:

user=>(deftype x [])
user.x
user=>(x? (x.))
true


 Comments   
Comment by Jozef Wagner [ 09/Oct/14 9:11 AM ]

What about camel cased types? predicate SomeType? does not look like an idiomatic type predicate. I suggest to have this type predicate function and its name optional, through e.g. :predicate metadata on a type name. Moreover, it is far more useful to have such predicate on protocols, rather than types.

Comment by Nicola Mometto [ 09/Oct/14 9:17 AM ]

I don't think camel cased types should pose any issue. we use ->SomeType just as fine, I don't see why SomeType? should be problematic.

I disagree that it's more useful to have a predicate for protocols since protocols are already regular Vars and it's just a matter of (satisfies? theprotocol x), the value of the predicate on types/record is to minimize the necessity of having to import the actual class

Comment by Pierre-Yves Ritschard [ 06/Jan/17 9:47 AM ]

This would be super useful, thanks.





[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-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-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-1575] Using a (def ^:const instance) of a deftype that implements IPersistentCollection, triggers compiler errors Created: 29/Oct/14  Updated: 24/Dec/16

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

Type: Defect Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

fresh repl


Attachments: Text File 0001-Test-for-analyzer-bug-CLJ-1575.patch    

 Description   

The compiler has a lot of assumptions about the possible types of IPersistentCollection literals and rightfully so. The strange thing with this case is, that taking the (constant) value works as soon as count is defined, but using it as an argument hits a closed dispatch for emitting the empty variants of the various literals.

> (deftype T [] clojure.lang.IPersistentCollection (count [_] 0)
> (def ^:const t (T.))
> (meta t)
java.lang.UnsupportedOperationException: Unknown Collection type
Compiler.java:2860 clojure.lang.Compiler$EmptyExpr.emit
Compiler.java:3632 clojure.lang.Compiler$InvokeExpr.emitArgsAndCall
...

EDIT updated the ticket after some investigation
NOTE attached test patch doesn't even implement (count []) for the deftype, which just triggers a rightful AbstractMethodError



 Comments   
Comment by Herwig Hochleitner [ 29/Oct/14 10:00 PM ]

The test had a typo, sorry

Comment by Alex Miller [ 30/Oct/14 7:14 AM ]

Looks like a variant of CLJ-1093.

Comment by Herwig Hochleitner [ 24/Dec/16 8:27 PM ]

This bug is still present in 1.8, even though CLJ-1093 has been marked fixed for 1.8.





[CLJ-2075] Add three-arities to < <= > >= = == not= Created: 03/Dec/16  Updated: 23/Dec/16

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: 5
Labels: None

Attachments: Text File clj-2075-add-three-arities-to-comparisons.patch     Text File clj-2075-over-clj-1912.patch    

 Description   

In my practice, using three-arities of less/greater operations is pretty common for e.g. checking a number is in range:

(< 0 temp 100)

The problem is, it is almost three times as slow compared to (and (< 0 temp) (< temp 100)).

This happens because three-arities are handled by the generic vararg arity branch:

(defn <
  "Returns non-nil if nums are in monotonically increasing order,
  otherwise false."
  {:inline (fn [x y] `(. clojure.lang.Numbers (lt ~x ~y)))
   :inline-arities #{2}
   :added "1.0"}
  ([x] true)
  ([x y] (. clojure.lang.Numbers (lt x y)))
  ([x y & more]
    (if (< x y)
     (if (next more)
       (recur y (first more) (next more))
       (< y (first more)))
     false)

This patch adds special handling for three-arities to these fns: < <= > >= = == not=

(defn <
  "Returns non-nil if nums are in monotonically increasing order,
  otherwise false."
  {:inline (fn [x y] `(. clojure.lang.Numbers (lt ~x ~y)))
   :inline-arities #{2}
   :added "1.0"}
  ([x] true)
  ([x y] (. clojure.lang.Numbers (lt x y)))
  ([x y z] (and (. clojure.lang.Numbers (lt x y))
                (. clojure.lang.Numbers (lt y z))))
  ([x y z & more]
   (if (< x y)
     (let [nmore (next more)]
       (if nmore
         (recur y z (first more) nmore)
         (< y z (first more))))
     false)))

The performance gains are quite significant:

(= 5 5 5) 	 24.508635 ns => 4.802783 ns (-80%)
(not= 1 2 3) 	 122.085793 ns => 21.828776 ns (-82%)
(< 1 2 3) 	 30.842993 ns => 6.714757 ns (-78%)
(<= 1 2 2) 	 30.712399 ns => 6.011326 ns (-80%)
(> 3 2 1) 	 22.577751 ns => 6.893885 ns (-69%)
(>= 3 2 2) 	 21.593219 ns => 6.233540 ns (-71%)
(== 5 5 5) 	 19.700540 ns => 6.066265 ns (-69%)

Higher arities also become faster, mainly because there's one less iteration now:

(= 5 5 5 5) 	 50.264580 ns => 31.361655 ns (-37%)
(< 1 2 3 4) 	 68.059758 ns => 43.684409 ns (-35%)
(<= 1 2 2 4) 	 65.653826 ns => 45.194730 ns (-31%)
(> 3 2 1 0) 	 119.239733 ns => 44.305519 ns (-62%)
(>= 3 2 2 0) 	 65.738453 ns => 44.037442 ns (-33%)
(== 5 5 5 5) 	 50.773521 ns => 33.725097 ns (-33%)

This patch also changes vararg artity of not= to use next/recur instead of apply:

(defn not=
  "Same as (not (= obj1 obj2))"
  {:tag Boolean
   :added "1.0"
   :static true}
  ([x] false)
  ([x y] (not (= x y)))
  ([x y z] (not (= x y z)))
  ([x y z & more]
   (if (= x y)
     (let [nmore (next more)]
       (if nmore
         (recur y z (first more) nmore)
         (not= y z (first more))))
     true)))

Results are good:

(not= 1 2 3 4) 	 130.517439 ns => 29.675640 ns (-77%)

I'm also doing what Jozef Wagner did in CLJ-1912 (calculating (next more) just once), although perf gains from that alone are not that big.

My point here is that optimizing three-arities makes sence because they appear in the real code quite often. Higher arities (4 and more) are much less widespread.



 Comments   
Comment by Nikita Prokopov [ 03/Dec/16 2:32 AM ]

Benchmark code here https://gist.github.com/tonsky/442eda3ba6aa4a71fd67883bb3f61d99

Comment by Alex Miller [ 03/Dec/16 8:24 AM ]

It might make more sense to combine this with CLJ-1912, otherwise these patches will fight.

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

Use this patch if CLJ-1912 would be applied first

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

I found a problem with previous patches that during defining = (equality), and is not yet defined. Replaced with if





[CLJ-1209] clojure.test does not print ex-info in error reports Created: 11/May/13  Updated: 22/Dec/16

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

Type: Enhancement Priority: Critical
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: clojure.test

Attachments: Text File 0001-use-new-printing-method.patch     Text File 0002-CLJ-1209-show-ex-data-in-clojure-test.patch     File clj-test-print-ex-data.diff     Text File output-with-0002-patch.txt    
Patch: Code
Approval: Triaged

 Description   

clojure.test does not print the data attached to ExceptionInfo in error reports.

(use 'clojure.test)
(deftest ex-test (throw (ex-info "err" {:some :data})))
(ex-test)

ERROR in (ex-test) (core.clj:4591)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ExceptionInfo: err
 at clojure.core$ex_info.invoke (core.clj:4591)
    user/fn (NO_SOURCE_FILE:2)
    clojure.test$test_var$fn__7666.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)
    ...

Approach: In clojure.stacktrace, which clojure.test uses for printing exceptions, add a check for ex-data and pr it.

After:

ERROR in (ex-test) (core.clj:4591)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ExceptionInfo: err
{:some :data}
 at clojure.core$ex_info.invoke (core.clj:4591)
    user/fn (NO_SOURCE_FILE:3)
    clojure.test$test_var$fn__7667.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)

Patch: 0002-CLJ-1209-show-ex-data-in-clojure-test.patch



 Comments   
Comment by Alex Miller [ 20/Dec/13 9:53 AM ]

Great idea, thx for the patch!

Comment by Alex Miller [ 20/Dec/13 9:54 AM ]

Would be great to see a before and after example of the output.

Comment by Ivan Kozik [ 12/Jul/14 10:35 PM ]

Attaching sample output

Comment by Stuart Sierra [ 05/Sep/14 3:24 PM ]

As pointed out on IRC, there's a possible risk of trying to print an infinite lazy sequence that happened to be included in ex-data.

To mitigate, consider binding *print-length* and *print-level* to small numbers around the call to pr.

Comment by Stephen C. Gilardi [ 13/May/15 2:39 PM ]

http://dev.clojure.org/jira/browse/CLJ-1716 may cover this well enough that this issue can be closed.

Comment by Alex Miller [ 14/May/15 8:35 AM ]

I don't think 1716 covers it at all as clojure.test/clojure.stacktrace don't use the new throwable printing. But they could! And that might be a better solution than the patch here.

For example, the existing patch does not consider what to do about nested exceptions, some of which might have ex-data. The new printer handles all that in a consistent way.

Comment by Ed Bowler [ 22/Dec/16 11:35 AM ]

I think http://dev.clojure.org/jira/secure/attachment/16361/0001-use-new-printing-method.patch fixes the printing of the Exceptions.





[CLJ-1951] bigint? predicate and generator Created: 08/Jun/16  Updated: 17/Dec/16

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: generator

Approval: Vetted

 Description   

Add bigint? and spec.gen support.

This part is easy:

(defn bigint?
  "Returns true if n is a BigInt"
  {:added "1.9"}
  [n] (instance? clojure.lang.BigInt n))

The generator is the tricky bit. test.check doesn't have a generator for bigints, just large-integer for things in long range. I think we'd want numbers beyond long range in a bigint generator (as that's a likely place where bugs might lie). Making a really high-quality bigint generator (with good growth and shrinking characteristics) is something that needs more thought.

http://clojure.github.io/test.check/clojure.test.check.generators.html#var-large-integer



 Comments   
Comment by Gary Fredericks [ 17/Dec/16 6:17 PM ]

In case I don't get around to making a patch, I think a generator along these lines would be a decent start:

(def gen-bigint 
  (gen/sized 
   (fn [size] 
     (let [large-integer (gen/resize size gen/large-integer)] 
       ;; scaling gives us relatively small vectors, but using  
       ;; the resized large-integer above means the numbers in 
       ;; the small vectors will still be big 
       (gen/scale #(+ 2 (/ % 20))
                  (gen/fmap (fn [xs] (+ (bigint (first xs)) (reduce * (map bigint (rest xs))))) 
                            (gen/not-empty (gen/vector large-integer))))))))
Comment by Gary Fredericks [ 17/Dec/16 6:21 PM ]

If anything seems inadequate about the sizing in the above generator, I should point out that sizing in test.check is rather subtle but the requirements are also not well-defined. I'd be happy to discuss in detail.





[CLJ-1293] Support (try .. (catch :default _ ..)) for portable "catch-all" Created: 05/Nov/13  Updated: 16/Dec/16

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: portability

Attachments: Text File CLJ-1293-v001.patch     Text File CLJ-1293-v002.patch     Text File CLJ-1293-v003.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Clojurescript has this to expose the untyped catch, which is equivalent to (catch Throwable _) on java.

http://dev.clojure.org/jira/browse/CLJS-661

Proposals

1) add (catch :default _) to mean (catch Throwable _)
2) add (catch :default _) to mean (catch Exception _)
3) add (catch :all _) to mean (catch Throwable _)

Please see design page for discussion of proposals: http://dev.clojure.org/display/design/Platform+Errors

Patches

v001 implements just 1)

This patch is more permissive than my patch for CLJS: The CLJS patch ensures :default catch blocks occur between non-default catch blocks and finally blocks, if present. This patch just makes (catch :default ...) a synonym for (catch Throwable ...). I wanted to keep the change to the compiler minimum.

Open Question: Catch Throwable (patch v001 does this) or Exception? Alternatively, a more carefully crafted list of "non-fatal" errors. See Scala's NonFatal pattern extractor: http://www.scala-lang.org/api/current/scala/util/control/NonFatal$.html

v002 implements 2) + 3)

This builds on v001, so the same caveat about clause ordering applies.

v003 implements just 2)

This builds on v001, so the same caveat about clause ordering applies.



 Comments   
Comment by Brandon Bloom [ 28/Dec/14 11:33 AM ]

Noticed this switched from "Minor" to "Critical", so I figured I should mention that I later realized that we might want :default to catch Exception instead of Throwable, so as to avoid catching Error subclasses. Javadocs say: "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch." If that's what we actually want, I can provide an updated patch.

Comment by Alex Miller [ 28/Dec/14 2:19 PM ]

Seems like an open question, might be best just to list it as such in the description.

I don't really expect to reach consensus on the ticket or patch right now, just trying to update priorities and raise visibility for discussion with Rich once we get to 1.8.

Comment by Herwig Hochleitner [ 07/Dec/16 4:15 PM ]

I'm in favor of catching Exception. It is the :default on java (as stated in the docs), so catching Throwable is a platform-specific thing to do and it would still be possible.

Comment by Herwig Hochleitner [ 07/Dec/16 5:08 PM ]

Hm, realizing now, that my last comment is at odds with the design discussion about being able to catch anything in javascript.

Attached patch v002 implements :all in addition to :default.

Comment by Herwig Hochleitner [ 07/Dec/16 6:30 PM ]

Realized, that catch-all vs catch-Exception is only a shallow contradiction: (catch Exception _) is - for all intents and purposes - the catch-all of java. Since the catch-absolutely-all is accessible in java through a regular catch, the driving need in clojurescript doesn't apply to clojure. The driving need in clojure is portability. In clojurescript, this is conflated with exposing an otherwise inaccessible platform feature, but that needs to not drive the general design.

Attached v003





[CLJ-1239] faster, more flexible dispatch for clojure.walk Created: 29/Jul/13  Updated: 14/Dec/16

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

Type: Enhancement Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 7
Labels: walk

Attachments: Text File 0001-CLJ-1239-protocol-dispatch-for-clojure.walk.patch     Text File 0002-CLJ-1239-protocol-dispatch-for-clojure.walk.patch    
Patch: Code
Approval: Triaged

 Description   

The conditional dispatch in clojure.walk is slow and not open to extension. Prior to CLJ-1105 it did not support records.

Approach: Reimplement clojure.walk using protocols. The public API does not change. Users can extend the walk protocol to other types (for example, Java collections) if desired. As in CLJ-1105, this version of clojure.walk supports records.

Patch: 0002-CLJ-1239-protocol-dispatch-for-clojure.walk.patch

Performance: My tests indicate this is 1.5x-2x the speed of the original clojure.walk. See https://github.com/stuartsierra/clojure.walk2 for benchmarks.

Risks: This approach carries some risk of breaking user code that relied on type-specific behavior of the old clojure.walk. When running the full Clojure test suite, I discovered (and fixed) some breakages that did not show up in clojure.walk's unit tests. See, for example, commit 730eb75 in clojure.walk2



 Comments   
Comment by Vjeran Marcinko [ 19/Oct/13 12:32 PM ]

It looks, as it is now, that walking the tree and replacing forms doesn't preserve original meta-data contained in data structures.

Comment by Andy Fingerhut [ 23/Nov/13 1:11 AM ]

Patch 0001-CLJ-1239-protocol-dispatch-for-clojure.walk.patch no longer applies cleanly to latest Clojure master since the patch for CLJ-1105 was committed on Nov 22, 2013. From the description, it appears the intent was either that patch or this one, not both, so I am not sure what should happen with this patch, or even this ticket.

Comment by Alex Miller [ 23/Nov/13 1:52 AM ]

This patch and ticket are still candidates for future release.

Comment by Stuart Sierra [ 20/Dec/13 9:14 AM ]

Added new patch that applies on latest master after CLJ-1105.

Comment by Chouser [ 27/Feb/14 10:26 AM ]

The way this patch behaves can be surprising compared to regular maps:

(clojure.walk/prewalk-replace {[:a 1] nil} {:a 1, :b 2})
;=> {:b 2}

(defrecord Foo [a b])
(clojure.walk/prewalk-replace {[:a 1] nil} (map->Foo {:a 1, :b 2}))
;=> #user.Foo{:a 1, :b 2}

Note how the [:a 1] entry is removed from the map, but not from the record.

Here's an implementation that doesn't suffer from that problem, though it does scary class name munging instead: https://github.com/LonoCloud/synthread/blob/a315f861e04fd33ba5398adf6b5e75579d18ce4c/src/lonocloud/synthread/impl.clj#L66

Perhaps we could add to the defrecord abstraction to support well the kind of things that synthread code is doing clumsily, and then walk could take advantage of that.

Comment by Alex Miller [ 27/Feb/14 2:11 PM ]

@Chouser, can you file a new ticket related to this? It's hard to manage work on something from comments on a closed ticket.

Comment by Alex Miller [ 27/Feb/14 3:54 PM ]

@Chouser - Never mind! I was thinking this was the change that went into 1.6. Carry on.

Comment by Nicola Mometto [ 27/Feb/14 5:17 PM ]

Alex, for what it matters clojure-1.6.0 after CLJ-1105 exibits the same behaviour as described by Chouser for this patch





[CLJ-308] protocol-ize with-open Created: 21/Apr/10  Updated: 14/Dec/16

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

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: io

Attachments: Text File 0001-Added-ClosableResource-protocol-for-with-open.patch    
Patch: Code
Approval: Triaged

 Description   

Good use (and documentation example) of protocols: make with-open aware of a Closable protocol for APIs that use a different close convention. See http://groups.google.com/group/clojure/browse_thread/thread/86c87e1fc4b1347c



 Comments   
Comment by Assembla Importer [ 24/Aug/10 4:39 PM ]

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

Comment by Tassilo Horn [ 23/Dec/11 5:11 AM ]

Added a CloseableResource protocol and extended it on java.io.Closeable (implemented by all Readers, Writers, Streams, Channels, Sockets). Use it in with-open.

All tests pass.

Comment by Tassilo Horn [ 23/Dec/11 7:14 AM ]

Seems to be related to Scopes (http://dev.clojure.org/jira/browse/CLJ-2).

Comment by Tassilo Horn [ 08/Mar/12 3:59 AM ]

Updated patch.

Comment by Andy Fingerhut [ 02/Apr/12 12:11 PM ]

Patch 0001-Added-ClosableResource-protocol-for-with-open.patch dated 08/Mar/12 applies, builds, and tests cleanly on latest master as of Apr 2 2012. Tassilo has signed a CA.

Comment by Tassilo Horn [ 13/Apr/12 11:23 AM ]

Updated patch to apply cleanly against master again.

Comment by Brandon Bloom [ 22/Jul/14 9:00 PM ]

I looked up this ticket because I ran in to a reflection warning: with-open does not hint it's binding with java.io.Closeable

Some feedback on the patch:

1) This is a breaking change for anyone relying on the close method to be duck-typed.

2) CloseableResource is a bit long. clojure.core.protocols.Closeable is plenty unambiguous.

3) Rather than extending CloseableResource to java.io.Closeable, you can use the little known (undocumented? unsupported?) :on-interface directive:

(defprotocol Closeable
  :on-interface java.io.Closeable
  (close [this]))

That would perform much better than the existing patch.

Comment by Tassilo Horn [ 23/Jul/14 7:12 AM ]

Hi Brandon, two questions:

Could 1) be circumvented somehow by providing a default implementation somehow? I guess the protocol could be extended upon Object with implementation (.close this), but that would give a reflection warning since Object has no close method. Probably one could extend upon Object and in the implementation search a "close" method using java.lang.reflect and throw an exception if none could be found?

Could you please tell me a bit more about the :on-interface option? How does that differ from extend? And how do I add the implementation, i.e., (.close this) with that option?





[CLJ-1527] Clarify and align valid symbol and keyword rules for Clojure (and edn) Created: 18/Sep/14  Updated: 14/Dec/16

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

Type: Enhancement Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 16
Labels: reader

Approval: Triaged

 Description   

Known areas of under-specificity (http://clojure.org/reader#The%20Reader--Reader%20forms):

  • symbols (and keywords) description do not mention constituent characters that are currently in use by Clojure functions such as <, >, =, $ (for Java inner classes), & (&form and &env in macros), % (stated to be valid in edn spec)
  • keywords currently accept leading numeric characters which is at odds with the spec - see CLJ-1286

References:



 Comments   
Comment by Andy Fingerhut [ 17/Oct/14 2:13 AM ]

The Clojure reader documentation also does not mention the following symbols as valid constituent characters. They are all mentioned as valid symbol constituent characters in the EDN readme here: https://github.com/edn-format/edn#symbols

dollar sign - used in Clojure/JVM to separate Java subclass names from class names, e.g. java.util.Map$Entry
percent sign - not sure why this is part of edn spec. In Clojure it seems only to be used inside #() for args like % %1 %&
ampersand - like in &form and &env in macro definitions
equals - clojure.core/= and many others
less-than - clojure.core/< clojure.core/<=
greater-than - clojure.core/> clojure.core/>=

I don't know whether Clojure and edn specs should be the same in this regard, but it seemed worth mentioning for this ticket.

Comment by Andy Fingerhut [ 01/Jun/15 12:22 AM ]

Alex, Rich made this comment on CLJ-17 in 2011: "Runtime validation off the table for perf reasons. cemerick's suggestion that arbitrary symbol support will render them valid is sound, but arbitrary symbol support is a different ticket/idea." I am not aware of any tickets that propose the enhancement of allowing arbitrary symbols to be supported by Clojure, e.g. via a syntax like

#|white space and arbitrary #$@)$~))@ chars here|

Do you think it is reasonable to create an enhancement ticket for supporting arbitrary characters in symbols and keywords?

Comment by Alex Miller [ 01/Jun/15 6:36 AM ]

Sure. I looked into this a bit as a digression off of feature expressions and #| has been reserved for this potential use. However, there are many tricky issues with it and I do not expect this to happen soon - more likely to be something we're pushed to do when necessary for some other reason.

Comment by Herwig Hochleitner [ 01/Jun/15 8:46 AM ]

Wrong ticket, but to anybody thinking about #|arbitrary symbols (or strings)|, please do consider making the delimiters configurable, as in mime multipart.

Comment by Andy Fingerhut [ 01/Jun/15 8:54 AM ]

I've created a design page for now. I'm sure it does not list many of the tricky issues you have found. I'd be happy to take a shot at documenting them if you have any notes you are willing to share.

http://dev.clojure.org/pages/viewpage.action?pageId=11862058

Comment by Andy Fingerhut [ 01/Jun/15 9:01 AM ]

Herwig, can you edit the design page linked in my previous comment, to add a reference or example to precisely how mime multipart allows delimiters to be configurable, and why you believe fixed delimeters would be a bad idea?

Comment by Herwig Hochleitner [ 01/Jun/15 9:46 AM ]

I've commented on the design page.

Comment by Alex Miller [ 13/Jul/15 12:44 PM ]

Removed a couple of issues that have been clarified on the reader page and are no longer issues.

Comment by Nicola Mometto [ 13/Jul/15 12:45 PM ]

Related to CLJ-1530

Comment by Adam Frey [ 15/Jul/15 11:55 AM ]

Related to this: The Clojure reader will not accept symbols and keywords that contain consecutive colons (See LispReader.java), although that is permitted by the current EDN spec. Here is a GitHub issue regarding consecutive colons. I would like to qualify why consecutive colons are disallowed, and sync up the Clojure Reader and the EDN spec on this.

Comment by Herwig Hochleitner [ 31/Jul/15 8:03 AM ]

The updated reader spec says that a symbol can contain a single / to separate the namespace. It also mentions a bare / to be the division function.
So what about clojure.core//? That still got to be a readable symbol right? So is that an exception to the 'single /' rule?
Will foo.bar// also be readable? What about foo//bar?

Comment by Francis Avila [ 10/Sep/15 9:26 AM ]

Another source of ambiguity I see is that it's unclear whether the first colon of a keyword is the first character of the keyword (and therefore of the symbol) or whether it is something special and the spec really describes what happens from the second character onward. This matters because the specification for a keyword is (in both edn and reader specs) given in terms of differences from symbols. I think many of the strange keyword edge cases (including legality of :1 vs :a/1) stem from this ambiguity, and different tickets/patches seem to choose one or the other underlying assumption. See this comment for more examples.

Possibly we can use tagged literals for keywords and symbols to create or print these forms when they are not readable and simplify the reader spec for their literal forms. E.g. instead of producing complicated parse rules to ensure clojure.core// or :1 are legal, just make the literal form simple and have users write something like #sym["clojure.core" "/"] or #kyw "1" (and have the printer print these) when they hit these edge cases.

Comment by Alex Miller [ 10/Sep/15 9:44 AM ]

I would say : (and : are syntactic markers and the spec describes the characters following it. But I agree it would be nice for this to be more explicit. The (incorrect) regex in LispReader does not help either.

The tagged literal idea is an interesting alternative to the | | syntax that has been reserved for possible future support for invalid characters in keywords and symbols. But I think the idea is out of scope for this ticket, which is really about clarifying the spec.

Comment by Steven Yi [ 08/Nov/16 11:16 AM ]

Coming to this late, I had mentioned on the user mailing list in:

https://groups.google.com/forum/#!topic/clojure/CwZHu1Eszbk

that # is currently allowed as part of a symbol name, such that:

(let a# 4 b#a 3 (println a# b#a))

will print "4 3".

  1. is also employed in auto-gensyms and discussed in http://clojure.org/reference/reader#syntax-quote as part of a symbol's name. From the mailing list thread, # was noted as "may be allowed now, but could be changed later". I would appreciate if it is more clearly described as a special case/reserved, and would ask that its use be restricted in the reader to prevent users from using it now and potentially have code break later.




[CLJ-1458] Enhance the performance of map merges Created: 04/Jul/14  Updated: 14/Dec/16

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

Type: Enhancement Priority: Major
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: performance

Attachments: Text File 0001-very-simple-test-of-the-merge-function.patch     Text File clj-1458-4.patch     Text File CLJ-1458-5.patch     Text File CLJ-1458-6.patch     Text File CLJ-1458-7.patch     Text File CLJ-1458-transient-merge2.patch     Text File CLJ-1458-transient-merge3.patch     Text File CLJ-1458-transient-merge.patch     Text File merge-test-2.patch     File transient-merge.diff    
Patch: Code and Test
Approval: Triaged

 Description   

It would be nice if merge used transients.

Patch

  • clj-1458-7.patch

Approach
Migrate c.c/merge later in core after transients & reduce. Leave older version as merge1 for use in cases the precede the newer definition. Make APersistentMap/conj & ATransientMap/cons aware of IKVReduce.

The attached patch preserves two existing behaviors of merge

  • metadata propagation
  • the right hand side of the merges can be a Map.Entry, an IPersistentVector where size=2, and regular maps.

Screened by:



 Comments   
Comment by Jason Wolfe [ 13/Sep/14 5:09 PM ]

I will take a crack at a patch today.

Comment by Jason Wolfe [ 13/Sep/14 5:42 PM ]

This patch (transient-merge.diff) makes merge, merge-with, and zipmap (since it was right there and could obviously benefit from transients as well) use transients.

Three potential issues:

  • I had to move the functions, since they depend on transient and friends. I assume this is preferable to a forward declaration. This was the best place I could find, but happy to move them elsewhere.
  • I added multiple arities, to avoid potential performance cost of transient-ing a single argument. Happy to undo this if desired.
  • I had to slightly alter the logic in merge-with, since transient maps don't support contains? (or find).
Comment by Michał Marczyk [ 14/Sep/14 12:43 PM ]

I posted a separate ticket for zipmap, with patch, on 30/May/12: CLJ-1005.

Comment by Jason Wolfe [ 14/Sep/14 5:28 PM ]

Ah, sorry if I overstepped then. Happy to remove that change from this patch then if that will simplify things – just let me know.

Comment by Ghadi Shayban [ 28/Dec/14 10:07 PM ]

alternate approach attached delaying merge until after protocols load, and then using transducers.

Comment by Michael Blume [ 28/Dec/14 11:50 PM ]

Looks like you're doing (get m k) twice – shouldn't that be thrown in a local?

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

um, put, in a local, I mean, 'throw' was a bad choice of word.

Comment by Ghadi Shayban [ 29/Dec/14 2:14 PM ]

Yeah there's that – won't be using get anyways after CLJ-700 gets committed.

We should add performance tests too. merging two maps, three, many maps, also varying the sizes of the maps, and for merge-with, varying the % of collisions.

Need to go back to the (some identity) logic, otherwise metadata is propagated from maps other than the first provided. I'll fix later.

Comment by Michael Blume [ 29/Dec/14 2:49 PM ]

I don't know if this is supposed to be allowed, but this breaks

(merge {} [:foo 'bar])

which is used in the wild by compojure-api

Comment by Michael Blume [ 29/Dec/14 2:49 PM ]

https://github.com/metosin/compojure-api/blob/0.16.6/src/compojure/api/meta.clj#L198

Comment by Michael Blume [ 29/Dec/14 2:54 PM ]

Ghadi, contains? uses get under the covers, so it's still two gets, right? It seems like it'd be more performant to stick with the ::none trick.

Comment by Nicola Mometto [ 29/Dec/14 5:36 PM ]

This calls for if-let + find.

Comment by Ghadi Shayban [ 29/Dec/14 10:37 PM ]

new patch addressing concerns so far

Comment by Ghadi Shayban [ 29/Dec/14 10:48 PM ]

CLJ-1458-transient-merge3.patch removes silly inlining macro, uses singleton fns instead.

Comment by Michael Blume [ 29/Dec/14 11:14 PM ]

Nice =)

This should come with tests. If we want to preserve the ability to merge with a MapEntry, we should test it. This isn't so much a weakness of the patch as of the existing tests. I see merge and merge-with being used a few times in the test suite, but I see no test whose purpose is to test their behavior.

Comment by Michael Blume [ 29/Dec/14 11:17 PM ]

Extremely simple merge test, we need more than this, but this is a start

Comment by Alex Miller [ 22/Jun/15 10:11 AM ]

clj-1458-4.patch refreshed to apply to master, no changes.

Comment by Ghadi Shayban [ 09/Jan/16 5:09 PM ]

I'd like to reevaluate the scope of this ticket. Can we address 'merge' only and defer 'merge-with'? It's by far the more common function. I've attached a new simplified patch.

Comment by Ghadi Shayban [ 09/Jan/16 9:50 PM ]

CLJ-1458-6.patch is yet another cleaner approach

Comment by Alex Miller [ 01/Feb/16 5:17 AM ]

Can you update the ticket approach section to discuss the APersistentMap.cons / ASSOC changes. Also, can you add a before / after perf test for one or more common cases?

Comment by Michael Blume [ 28/Sep/16 1:55 PM ]

Updated patch to handle use of merge in core_print before it's defined in core

Comment by Ghadi Shayban [ 28/Sep/16 2:22 PM ]

If anyone wants to take stewardship of this, go ahead. I had trouble getting consistent performance improvements on this. Obviously this needs fresh benchmarks.

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

Yes, this needs a benchmark showing demonstrable improvement. The whole goal here is improved perf - if we can't prove it's consistently faster, then there is no point in even reviewing it.





[CLJ-2054] generator for `any?` occasionally generates `Double/NaN` for which equality semantics don't apply, and that is a problem for the :ret spec of many functions. Created: 07/Nov/16  Updated: 14/Dec/16

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

Type: Enhancement Priority: Major
Reporter: Dimitrios Piliouras Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: generator, spec
Environment:

Ubuntu 16.10 - Oracle Java 8


Approval: Triaged

 Description   

The generator for `any?` will occasionally give back Double/NaN value(s). Since NaNs & equality (via `=`) don't get along, :ret spec'ing a fn which transforms/processes a collection according to a predicate, becomes rather problematic. That's because the most obvious thing to check under :ret (the case where the predicate didn't return true for any value, and so the output coll should be equal to the input coll because nothing was transformed/processed), cannot be expressed trivially.

The workaround I've come up with in my own specs is to spec the elements of the collection with `(s/and any? (complement double-NaN?))` instead of just `any?`, and it works. However, even though I can live without NaNs in the tests, I must admit it still feels sort of hacky.

Ideas:

1) The generator for `any?` could be hardcoded to never return Double/NaN. Sounds rather invasive.
2) The generator for `any?` could be reworked to somehow be configurable wrt allowing/prohibiting Double/NaNs. Then perhaps a dynamic-var and/or a macro (e.g. `without-NaNs`) could expose this (just brainstorming here).
3) The generator for `any? could stay as is, but a new equality operator could be added (e.g. `clojure.spec/===`), which somehow ignores NaNs (a naive implementation for instance might walk the data-structures and replace all NaNs with keywords, and only then perform a regular comparison).



 Comments   
Comment by Alex Miller [ 08/Nov/16 10:29 AM ]

Should consider whether this change is more appropriate in test.check or in the spec generator for any?.

Comment by Dimitrios Piliouras [ 11/Nov/16 12:31 PM ]

It turns out that my workaround does not fully work. I literally just stumbled in the following case:

{nil {[] {NaN 0}}}

which is a conforming value for:

(s/def ::persistent-map
(s/map-of ::anything-but-NaN ::anything-but-NaN)) ;; (s/and any? (complement double-NaN?))

So basically, the inner collections can still have NaNs. So far I've got 4 specs that I've written and faced this problem on all of them.





[CLJ-2083] spec for printable/readable/edn data Created: 12/Dec/16  Updated: 12/Dec/16

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

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


 Description   

When spec'ing some things, I've used `any?` in a few cases where it is overly permissive. In particular, sometimes I need to specify a value must be printable/readable, such as when a value may wind up in an edn file. Similarly, I've needed to spec something must have non-generative value-identity, ie. ban closures, etc. Printable/readable or simply `edn?` would be a much better approximation than `any?`.



 Comments   
Comment by Brandon Bloom [ 12/Dec/16 1:08 PM ]

I realize that an edn? predicate would have O(N) runtime, vs an edn spec that could take advantage of every/every-kv etc for sampling conformance.

Comment by Herwig Hochleitner [ 12/Dec/16 11:12 PM ]

Related: CLJ-1527





[CLJ-2082] Improve documentation of clojure.walk/walk Created: 11/Dec/16  Updated: 11/Dec/16

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

Type: Enhancement Priority: Minor
Reporter: David Cook Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docs, docstring, documentation


 Description   

The documentation for the clojure.walk module isn't clear on which methods recurse through data structures, and which only operate on the outermost layer. The documentation for clojure.walk/walk and clojure.walk/postwalk both use forms of the word "traverse," and there's nothing calling out that clojure.walk/walk, unlike the rest of the functions in the namespace, doesn't recurse through the provided form.






[CLJ-2081] for macro spec should know :let can't go in the first position Created: 09/Dec/16  Updated: 09/Dec/16

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

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

Approval: Triaged

 Description   

The for macro does not support :let in the first position. This was reported in CLJ-207 and a patch was produced but rejected. Since we have spec for macros now, it might be an opportunity to provide a better error message. (I think first-place let should be fine, but that's neither here nor there.)






[CLJ-2079] Generator overrides for spec aliases are not respected Created: 08/Dec/16  Updated: 08/Dec/16

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

Type: Defect Priority: Major
Reporter: Nate Smith Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: generator, spec

Approval: Vetted

 Description   

Generator overrides for spec aliases are not respected.

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(require '[clojure.spec :as s])
(require '[clojure.spec.gen :as gen])
(s/def ::original number?)
(s/def ::alias ::original)

(every? double? (gen/sample (s/gen ::alias {::alias gen/double})))
;; => false

Providing a generator override for the original spec works as expected:

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(every? double? (gen/sample (s/gen ::alias {::original gen/double})))
;; => true


 Comments   
Comment by Alex Miller [ 08/Dec/16 5:02 PM ]

Probably a missing delay in the alias case - there's another ticket that has the same cause.

Comment by Nate Smith [ 08/Dec/16 6:43 PM ]

Looks like it might be because gensub looks for matching overrides by calling spec-name, which returns the wrong value for spec aliases.

(require '[clojure.spec :as s])
(s/def ::original number?)
(s/def ::alias ::original)
(@#'clojure.spec/spec-name (s/get-spec ::alias))
;; => :user/original




[CLJ-1912] Optimized version of the '<' and '>' functions for arties larger than 2 Created: 08/Apr/16  Updated: 03/Dec/16

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

Type: Enhancement Priority: Minor
Reporter: Anton Harald Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: performance

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

 Description   

When looking at the code of the build-in functions '<' and '>', (next more) is invoked twice in each comparison of two neighboring arguments.

Proposed: Compute (next more) only once per 'iteration' for the following set of functions that have the same code pattern: =, ==, <, <=, >, >=.

Perf improvements (see comments for more details):

Function Arity Before After % improved
< 4 140.7 ns 141.6 ns -0.6%
< 10 505.3 ns 460.3 ns 8.9%
< 100 9.0 µs 8.6 µs 4.4%
< 10000 885 µs 851 µs 3.8%
= 4 86.1 ns 86.8 ns 0.8%
= 10 333.4 ns 300.6 ns 9.8%
= 100 4.28 µs 3.65 µs 14.7%
= 10000 397.4 µs 353.3 µs 11.0%
== 4 138.1 ns 135.7 ns 1.7%
== 10 487.9 ns 460.9 ns 5.5%
== 100 5.58 µs 5.27 µs 5.6%
== 10000 565.0 µs 537.7 µs 4.8%

Patch: clj-1912.patch

Prescreened by: Alex Miller



 Comments   
Comment by Alex Miller [ 08/Apr/16 4:23 PM ]

I don't think there is a particular reason, feel free to make a patch.

Comment by Jozef Wagner [ 01/Dec/16 2:19 PM ]

Patch added. As if-let is defined later in the file, a combination of let and if is used.

Comment by Alex Miller [ 01/Dec/16 8:22 PM ]

Can you include benchmark code and benchmarks?

Comment by Jozef Wagner [ 02/Dec/16 3:25 AM ]

Benchmarks for <

(bench (< 1 2 3 4)) ;; 140.726376 ns
(bench (new< 1 2 3 4)) ;; 141.661964 ns

(bench (< 1 2 3 4 5 6 7 8 9 10)) ;; 505.381596 ns
(bench (new< 1 2 3 4 5 6 7 8 9 10)) ;; 460.331840 ns

(bench (apply < (range 100))) ;; 9.020666 µs
(bench (apply new< (range 100))) ;; 8.604638 µs

(bench (apply < (range 10000))) ;; 885.361898 µs
(bench (apply new< (range 10000))) ;; 851.344031 µs

Benchmarks for =

(bench (= 1 1 1 1)) ;; 86.114371 ns
(bench (new= 1 1 1 1)) ;; 86.874012 ns

(bench (= 1 1 1 1 1 1 1 1 1 1)) ;; 333.438530 ns
(bench (new= 1 1 1 1 1 1 1 1 1 1)) ;; 300.628516 ns

(bench (apply = (repeat 100 1))) ;; 4.282752 µs
(bench (apply new= (repeat 100 1))) ;; 3.650438 µs

(bench (apply = (repeat 10000 1))) ;; 397.401808 µs
(bench (apply new= (repeat 10000 1))) ;; 353.294723 µs

Benchmarks for ==

(bench (== 1 1 1 1)) ;; 138.162620 ns
(bench (new== 1 1 1 1)) ;; 135.759047 ns

(bench (== 1 1 1 1 1 1 1 1 1 1)) ;; 487.963993 ns
(bench (new== 1 1 1 1 1 1 1 1 1 1)) ;; 460.982411 ns

(bench (apply == (repeat 100 1))) ;; 5.587064 µs
(bench (apply new== (repeat 100 1))) ;; 5.273621 µs

(bench (apply == (repeat 10000 1))) ;; 565.031286 µs
(bench (apply new== (repeat 10000 1))) ;; 537.789795 µs

Benchmark code





[CLJ-1952] include var->sym in clojure.core Created: 08/Jun/16  Updated: 02/Dec/16