[CLJ-2484] Significant performance regression of code loaded in user.clj in Java 8u202 / 11.0.2 Created: 16/Feb/19  Updated: 21/Feb/19

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: performance, regression
Environment:

Java 8u201 and Java 11.0.2


Attachments: Text File clj-2484-1.patch     Text File clj-2484-nested-class.patch    
Patch: Code
Approval: Triaged

 Description   

The recent releases of Java 8u201/8u202 and 11.0.2 include a fix for a 0-day vulnerability that could occur while running a class static initializer. The fix included in these releases prevents JVM optimization during the static initializer (before the class is initialized). Doing significant work in the static initializer can see dramatic reduction in performance from the previous versions 8u192 / 11.0.1.

Clojure supports a well-known file user.clj that is loaded during runtime initialization (in the clojure.lang.RT class). This is sometimes used to load tools during development. Currently user.clj is loaded in the scope of RT's static inititializer, so any Clojure code run in user.clj is significantly slower.

One common pattern popularized by Stuart Sierra is the "reloaded" pattern which will load and initialize the entire server from the user.clj. Users of this pattern are likely to see dramatic startup time performance issues with these Java releases.

Links:

Repro: see https://github.com/puredanger/slow-user-load

Options:

1) One option is to move the doInit() logic out of the RT static initializer and then force explicit initialization of RT in all expected RT entry points (clojure.main, Clojure, genclass, AOT class, etc). This is pretty straightforward but has the major question of knowing all the entry points. There's a relatively small number of known entry points that covers a large percentage of users. Unfortunately, finding the remaining ones is tough.

  • clj-2484-1.patch - first cut, almost certainly incomplete, but this works.

2) Another option is to take the hints from the scope of the issue at https://cl4es.github.io/2019/02/21/Cljinit-Woes.html and avoid calling static methods on an uninitialized class. The basic idea here is to move all of the RT static methods into an inner class RT.Impl. That class can fully load so all methods are initialized. The RT static init can then just invoke doInit() on it. All references to RT static methods in Clojure must be rerouted from RT to RT.Impl. The existing RT methods are left and forward to RT.Impl - this allowed previously compiled Clojure classes to continue to work (or any stray RT calls made by advanced Clojure code).

  • clj-2484-nested-class.patch - this works, but is an enormous patch as it touches all of RT and every call to RT in Clojure.

3) Do nothing and wait for Java to mitigate the perf impact.



 Comments   
Comment by Alex Miller [ 20/Feb/19 1:15 PM ]

The Java bug http://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8219233 is accumulating a lot of additional information if you are looking to follow updates on the Java side. Seems to be narrowing specifically to static initializers that make calls to static methods in same class (this is particularly why user.clj loading in RT.init is impacted - loading all that Clojure code makes tons of self calls into the runtime).





[CLJ-2487] Implement completion/exception handling callbacks for promises and futures Created: 21/Feb/19  Updated: 21/Feb/19

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

Type: Enhancement Priority: Major
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Based on https://dev.clojure.org/display/design/Promises

Discussion on Slack:

Enhance `future`/`promise` to be able to call a function on completion/failure (I'd use promises a lot more if I had this functionality but you can sort of hack around it in ugly ways with futures and/or additional functions/macros).






[CLJ-2347] [spec] 'inst?' spec generator produces unreadable instants Created: 15/Apr/18  Updated: 20/Feb/19

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

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

Clojure 1.9.0
org.clojure/spec.alpha 0.1.143
org.clojure/test.check 0.9.0


Approval: Triaged

 Description   

The spec generator associated with inst? may produce instants that are technically valid but cannot be read back (and also are not practical in reality).

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

(second (last (s/exercise inst? 100)))
;; => #inst "883641-02-19T16:17:26.482-00:00"

#inst "883641-02-19T16:17:26.482-00:00"
;; => RuntimeException Unrecognized date/time syntax: 883641-02-19T16:17:26.482-00:00  clojure.instant/fn--7987/fn--7988 (instant.clj:107)

Minor issue, but I ran into this while interacting with inst generator/generated values in the REPL.



 Comments   
Comment by David Bürgin [ 12/May/18 3:53 AM ]

This problem gets worse when the inst generated is in the BC era. In that case, not even can the inst potentially not be read back, but since there is no indication of the ‘negative year’, information is lost.

Comment by Enzzo Cavallo [ 20/Feb/19 7:20 AM ]

There is some info about the limits of reader/printer in CLJ-2486
I catch this problem because I'm generating data and `pr-str` / `read-string` over the wire.





[CLJ-2464] peek does not support transient vectors Created: 07/Jan/19  Updated: 19/Feb/19

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

Type: Defect Priority: Major
Reporter: yhq Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: transient

Approval: Triaged

 Description   

Appears to be an oversight, given that peek is a read operation.

Input:

(peek (transient [:a :b :c]))

Expected outcome:

:c

Actual outcome:

Unhandled java.lang.ClassCastException
clojure.lang.PersistentVector$TransientVector cannot be cast to clojure.lang.IPersistentStack


 Comments   
Comment by Steve Miner [ 19/Feb/19 2:16 PM ]

I just ran into this issue today. I agree it would be convenient if peek worked on a transient vector. Other functions such as nth and count work on transient vectors, so it's natural to want peek.

As a work-around (not a suggested fix), I use this:

(defn peek! [tv] (nth tv (dec (count tv))))

Admittedly, that's a misnomer but it fits the bang pattern for transient transformation of regular collection code. I also have a convenience function update! which calls assoc!. I offer the work-around just for users who run into this problem and want to get back to work.

Comment by Alex Miller [ 19/Feb/19 4:40 PM ]

I think this would mean PersistentVector.TransientVector would need to support IPersistentStack but that has two methods - peek and pop. Interestingly, ITransientVector and TransientVector both have pop() already. Seems like the pop() impl has all the smarts you'd need to implement peek() too.





[CLJ-2485] BigDecimal loses M suffix when converted to string using (str) Created: 17/Feb/19  Updated: 18/Feb/19

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

Type: Defect Priority: Minor
Reporter: Mikhail Gusarov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Triaged

 Description   

(str 1M) produces "1"
(str {:a 1M}) procuces "{:a 1M}"

The suffix is lost because str calls .toString directly: java.lang.BigDecimal does not know anything about M suffix.

`.toString` on map calls RT.print and that recursively calls RT.print for keys and values. RT.print has a special case for java.lang.BigDecimal, so it prints the suffix.



 Comments   
Comment by Alex Miller [ 17/Feb/19 4:10 PM ]

If you want to print a string that can later be read, normally you use pr-str. Some reason that’s not better here?

str intentionally does not have the expectation to be readable.

Comment by daniel sutton [ 17/Feb/19 4:20 PM ]

This was discovered originally investigating `spit`. This particular behavior turned out not to be the culprit but was something noticed while investigating.

clojure -e '(spit "foo.edn" 3M)' && cat foo.edn
3

clojure -e '(spit "foo.edn" {:thing 3M})' && cat foo.edn
{:thing 3M}

We were wondering if using spit for edn was a footgun or if it was a genuine bug.

Comment by Mikhail Gusarov [ 17/Feb/19 4:21 PM ]

spit calls str, that's how I stumbled upon it.

It's trivial to make a pr-spit that calls pr-str, but isn't then spit kind of useless if its results can't be {{slurp}}ed back?

Comment by Alex Miller [ 18/Feb/19 12:15 PM ]

Oh, well that’s considerably more interesting. As a workaround you could pr-str before you spit.

The question of edn printing is one that has come up several times (there might even be a ticket about it).

Comment by Alex Miller [ 18/Feb/19 12:18 PM ]

CLJ-1201 is one such ticket





[CLJ-2483] [spec] generator in keys* spec not overridable Created: 16/Feb/19  Updated: 16/Feb/19

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

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


 Description   

Problem:

A generator in a keys* spec not overridable.

Repro:

(require '[clojure.spec.alpha :as s])
(require '[clojure.spec.gen.alpha :as gen])
(s/def ::foo (s/keys* :req-un [::a ::b]))
(s/def ::a number?)
(s/def ::b number?)
(s/valid? ::foo [:a 1 :b 2]) ;; true
(gen/generate (s/gen ::foo {::a (fn [] (gen/return 1))})) ;;=> (:a -15164 :b 24.0)

The last expression should return a sequence where the key :a is followed by 1 like: (:a 1 :b 24.0).

This may be traced back to a more general problem: overriding the generator on a spec that's made with with-gen seems to have no effect:

(gen/generate
   (s/gen (s/with-gen ::foo
            (fn []
              (s/gen (s/cat :key-a #{:a} :val-a ::a :key-b #{:b} :val-b ::b))))
          {::a (fn [] (gen/return 1))})) ;;=> (:a -1 :b "OXi2")





[CLJ-2433] Invalid calls to clojure.set functions return an incorrect answer rather than error Created: 16/Nov/18  Updated: 15/Feb/19

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

Type: Enhancement Priority: Major
Reporter: Erik Assum Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Approval: Triaged

 Description   

There are a number of tickets concerned with the fact that the set functions in clojure.set misbehave when passed arguments that are not sets.

This set of issues include CLJ-810, CLJ-1087, CLJ-1682, and CLJ-1954

The functions affected by this are:

  1. difference
  2. intersection
  3. union
  4. subset?
  5. superset?

as these are known to produce unexpected results when passed non-set arguments.

Problem
As the above mentioned issues suggest, todays implementation of these functions leads to confusion and erroneous results when called with non-set input. The user is given no warning or indications of the error he's making.

Possible solutions

  1. Add a coercion to set on the arguments said functions
  2. Throw an exception when the arguments are not sets
  3. Handle this with clojure.spec
  4. Leave it as is

Tradeoffs

  1. Given CLJ-2362, which makes a call to set close to a noop, the coercion should not incur much of a performance penalty. It has been argued that the code might even be faster, as type hints can be given and the compiler/jit might make better choices. For the common mistakes (passing vectors/lists instead of sets) it should be backwards compatible
  2. Throwing an exception on non-set arguments would break programs which work correctly today (although by chance), such as data.diff.
  3. Handling it with clojure.spec seems like a viable option, but again, this would break data.diff if the functions were spec'ed to both receive and return sets.
  4. Leaving it as it is, and we will continue to surprise both new and old clojurists.

Evidence of this being a problem

  1. The tickets mentioned above seem to indicate that people stumble upon this often enough to file issues
  2. https://clojuredocs.org/clojure.set/superset_q#example-5b5acd38e4b00ac801ed9e39


 Comments   
Comment by Alex Miller [ 16/Nov/18 1:34 PM ]

Before being considered, this ticket needs:

  • a good problem statement
  • an assessment of where existing code calls these functions with something other than sets
  • a table of alternatives and their tradeoffs. presumably alternatives are: add specs, add validation checks, add coercions, etc. Tradeoffs may include effects on existing callers (known or unknown), performance, etc.

Decisions to make:

  • Are existing calls (with inputs that are not sets) valid or invalid?
  • What change should be made in the functions

Patches are probably not super useful until those things are decided.

Comment by Michiel Borkent [ 17/Nov/18 6:52 AM ]

Interesting data from a repo by Andy Fingerhut comparing the performance of:

  • versions of set functions with pre-conditions (no coercion)
  • instrumented set functions

The functions with pre-conditions are significantly faster than the instrumented ones, but not much slower than the originals.

https://github.com/jafingerhut/funjible#wait-isnt-this-what-clojurespec-is-for

Comment by Stuart Halloway [ 18/Nov/18 7:40 AM ]

A spec test could compare inputs and outputs and not break any existing code.

Comment by Jan Rychter [ 15/Feb/19 3:20 PM ]

I'll attempt a first (minimal) step here. Since set functions have docstrings that begin with "Return a set", one can reasonably expect that they will always return a set.

The specific case that I encountered was `(clojure.set/difference nil #{1})` returning nil. The reason why the nil appeared was because of optionality: a set operation was performed and the source data was optional in the map. Data passed spec validations (because the key was optional), and only later tripped other validations because of the nil returned by set/difference. I realize that the arguments are incorrect and I do not necessarily expect auto-coercion.

What I would expect is a post-condition error that would show up in code compiled with assertions.

Specifically, I think a `{:post [(set? %)]}` post-condition on set functions is something that can be agreed upon. It doesn't address coercion, and it avoids the discussion of what are valid arguments, it just places a restriction on the return value according to documentation.

Given what the docstrings say, I do not think there is code out there that relies on set functions returning anything else than a set.

The performance impact of a post-condition would only be present if compiled with assertions.

I realize this doesn't address all issues mentioned, but it's perhaps a way to start moving forward.

I do not think this is a "Major" issue as currently marked.





[CLJ-1187] Clojure loses quoted metadata on empty literals Created: 22/Mar/13  Updated: 15/Feb/19

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: collections, metadata

Attachments: Text File 0001-Support-retrieval-of-metadata-from-quoted-empty-lite.patch     Text File 001-CLJ-1187.patch    
Patch: Code and Test
Approval: Vetted

 Description   

meta on empty collections is lost:

user=> (meta '^:foo [])
nil   ;; expected {:foo true} as in:
user=> (meta '^:foo [1])
{:foo true}

This bug propagates to ^:const vars:

user=> (def ^:const foo ^:foo [])
#'user/foo
user=> (meta foo)
nil
user=> (meta @#'foo)
{:foo true}

Cause: As in CLJ-1093, empty collections are replaced with an EmptyExpr that loses meta

Proposed: Don't replace with EmptyExpr if meta is present.

Patch: 0001-Support-retrieval-of-metadata-from-quoted-empty-lite.patch

Screened by:



 Comments   
Comment by Nicola Mometto [ 22/Mar/13 2:12 PM ]

After patch:

user=> (meta '^:foo [])
{:foo true}
user=> (meta '^:foo [:a])
{:foo true}
user=> (def ^:const foo ^:foo [])
#'user/foo
user=> (meta foo)
{:foo true}

Comment by Stuart Halloway [ 29/Mar/13 6:13 AM ]

I believe the title should read "Clojure loses quoted metdata on empty literals".

Comment by Stuart Halloway [ 29/Mar/13 6:16 AM ]

At first glance, the implementation looks wrong in that it blocks non-IObjs ever getting to EmptyExpr. Probably all persistent collections are IObjs, but would not want to bake this in.

Comment by Nicola Mometto [ 29/Mar/13 7:00 AM ]

You're right, I've updated my patch, it should work as expected now

Comment by Andy Fingerhut [ 04/Apr/13 9:44 PM ]

Nicola: Your updated patch 001-CLJ-1187.patch dated Mar 29, 2013 gives syntax errors when I try to compile it.

Comment by Nicola Mometto [ 05/Apr/13 9:06 AM ]

Oh, the irony, I messed up some parentheses writing java.

Sorry for it, here's the correct patch, it applies on upstream/master.

Comment by Gabriel Horner [ 24/May/13 9:14 AM ]

Looks good

Comment by Nicola Mometto [ 26/Jun/13 8:08 PM ]

CLJ-1093 contains a patch that fixes this issue aswell and should be preferred

Comment by Alex Miller [ 18/Aug/13 2:46 PM ]

Marking un-Screened due to the note about CLJ-1093. Want to assess this more before going through Rich.

Comment by Alex Miller [ 18/Oct/13 8:49 AM ]

Switching to Incomplete pending CLJ-1093 which I hope to pull into 1.6.

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

Pulling out of 1.6, will consider in next release.

Comment by Alex Miller [ 06/Jul/14 6:25 PM ]

Dupe of CLJ-1093, closed at Nicola's request.

Comment by Nicola Mometto [ 13/Dec/18 10:51 AM ]

Reopening this since the fix that got applied for CLJ-1093 didn't subsume this one

Comment by Nicola Mometto [ 13/Dec/18 10:52 AM ]

Refreshed and squashed patch

Comment by Shogo Ohta [ 15/Feb/19 4:04 AM ]

Any chance this will be fixed yet? I've run into it recently.

Comment by Alex Miller [ 15/Feb/19 7:36 AM ]

It’s been vetted by Rich so it’s in the path to consideration.

Comment by Shogo Ohta [ 15/Feb/19 7:52 AM ]

Yey! I hope things will go smoothly





[CLJ-1472] The locking macro fails bytecode verification on ART runtime Created: 23/Jul/14  Updated: 15/Feb/19

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

Type: Defect Priority: Minor
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: android, graal
Environment:

Android ART runtime


Attachments: Text File 0001-CLJ-1472-Locking-macro-without-explicit-monitor-ente.patch     Text File clj-1472-2.patch     Text File clj-1472-3.patch     Text File clj-1472.patch    
Patch: Code
Approval: Screened

 Description   

Android ART runs compile time verification on bytecode and fails on any usage of the locking macro. The error looks like that seen in CLJ-1829 (in that case clojure.core.server/stop-server calls the locking macro):

10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: java.lang.VerifyError: Rejecting class clojure.core.server$stop_server because it failed compile-time verification (declaration of 'clojure.core.server$stop_server' appears in /data/app/com.clojure_on_android-1/base.apk)

Cause: From discussion on an Android issue (https://code.google.com/p/android/issues/detail?id=80823), it seems this is due to more strictly enforcing the "structural locking" provisions in the JVM spec (https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-2.html#jvms-2.11.10).

It appears that the mixture of monitor-enter, monitor-exit, and the try/finally block in `locking` create paths that ART is flagging as not having balanced monitorenter/monitorexit bytecode. Particularly, monitorenter and monitorexit themselves can throw (on a null locking object). The Java bytecode does some tricky exception table handling to cover these cases which (afaict) is not possible to do without modifying the Clojure compiler.

Approach: One possible approach is to have locking invoke the passed body in the context of a synchronized block in Java. This avoids the issue of the tricky bytecode by handing it over to Java but has unknown performance impacts.

Patch: clj-1472-3.patch

See also: Examination of the bytecode as compared to a java synchronized block shows up a number of differences:
https://gist.github.com/AdamClements/2ae6c4919964b71eb470

Screened by: Alex Miller - I'm marking this screened as I think it is a viable approach that fixes the issue and due to the infrequency of its use, I'm not really that concerned about it being a performance problem. I will flag that I think another way to handle this would be to make `locking` a special form with compiler support, but I'm not sure whether that's worth doing or not, so I will leave that to Rich to decide.



 Comments   
Comment by Adam Clements [ 24/Jul/14 11:17 AM ]

After using this a little more, I've found that moving this outside the try block breaks nREPL.

Looking at the bytecode, the monitorenter for the locking in clojure.tools.nrepl.middleware.session/session-out and in a few other places ends up in an entirely different method definition and we now get a JVM IllegalMonitorStateException as well as an ART verification error for this function.

Comment by Andy Fingerhut [ 01/Aug/14 9:08 PM ]

Adam, I cannot comment on whether your patch is of interest or not, but it is true that no patch will be committed to Clojure if the author has not signed a Contributor Agreement, which can now be done on-line at http://clojure.org/contributing

Comment by Adam Clements [ 04/Aug/14 4:24 PM ]

Uploaded a new patch (and signed the contributor agreement). This passes both the JVM and ART bytecode verification, The extra try/catch around the monitor exit is optional (verification passes with or without it) but where the java version retries monitor-exit indefinitely and shows the deadlock at the right time, without catching errors in the monitor-exit an undetermined monitor-enter in the future might fail, not showing up the actual bug.

It's not very pretty, but without finer grained control of the generated bytecode, this is the best I could do.

Comment by Adam Clements [ 25/Nov/14 8:31 AM ]

Have just tested with Lollipop, and this patch might no longer be sufficient.

Getting in touch with the ART guys to see if they can shed a little more light and verify whether it will work on the current master branch of AOSP

Comment by Adam Clements [ 25/Nov/14 9:49 AM ]

Bug filed with AOSP project, hopefully they can shed some light on whether it is our problem and if so how we can fix it.

https://code.google.com/p/android/issues/detail?id=80823

Comment by Adam Clements [ 28/Nov/14 11:03 AM ]

I have uploaded an alternative implementation of the locking macro (0001-CLJ-1472-Locking-macro-without-explicit-monitor-ente.patch) which cheats a little - the synchronized block is actually implemented in Java and so guarantees compatibility. This is at the cost of a little extra indirection and the naming/location could probably be better.

But it does fix the bug and work on all versions of android, android + art and the jvm. Would this approach be acceptable?

Comment by Kevin Downey [ 08/Dec/14 1:12 PM ]

I have yet to see any evidence that the bytecode clojure is generating in some way violates the jvm spec, so I suspect the issue is clojure requires a jvm to run, and android doesn't provide a jvm, just something that looks like one if you don't tread outside the beaten path.

Comment by Kevin Downey [ 08/Dec/14 1:27 PM ]

given the structured locking verbiage in https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-2.html#jvms-2.11.10, `(locking nil)` may generate bytecode whose runtime behavior violates structured locking. the first patch on this issue can cause the compiler to emit monitorenter/exit instructions in different methods, which definitely violates structured locking

Comment by Adam Clements [ 09/Dec/14 10:45 AM ]

Yes, the first patch was definitely wrong, I left it for some context to the conversation, but it's probably best to just remove it for clarity.

For anyone following this conversation who doesn't want to decompile and observe the bytecode, here's a gist with the difference between a java synchronized block and clojure locking https://gist.github.com/AdamClements/2ae6c4919964b71eb470

I'm finding it hard to work out where the deviation from the spec occurs too, though I can see the difference with the Java version, if anything, the Clojure version looks closer to what's described in the spec than the Java one!

If someone with more knowledge than me on the subject could engage on the AOSP bug https://code.google.com/p/android/issues/detail?id=80823 then perhaps we could settle this as an android bug which is too focussed on the java implementation rather than the JVM spec, or perhaps they'll find something that's wrong with the Clojure implementation. I have uploaded the original clojure behaviour and asked them for some more explanation on why it is failing.

Comment by Adam Clements [ 09/Dec/14 11:09 AM ]

The response from the ART guys about what they think we're violating is:

The section on "Structured locking" contains the following:

"[...] implementations [...] are permitted but not required to enforce
both of the following two rules guaranteeing structured locking. [...]"

ART currently enforces both rules at verification time, including

"At no point during a method invocation may the number of monitor exits
performed by T on M since the method invocation exceed the number of
monitor entries performed by T on M since the method invocation."

Comment by Adam Clements [ 09/Dec/14 11:32 AM ]

If for example instruction https://gist.github.com/AdamClements/2ae6c4919964b71eb470#file-test_locks-class-L24 or the monitor-enter itself on the next line were to fail, couldn't it could end up in the finally clause and attempt to release the lock even though it has never been captured?

I think this violates the structured locking rules in the jvm spec you linked to.

Comment by Kevin Downey [ 09/Dec/14 1:15 PM ]

an interesting question about structured locking, is does the specification refer to the static bytecode or the runtime behavior of the bytecode. given the bytecode linked(https://gist.github.com/AdamClements/2ae6c4919964b71eb470#file-test_locks-class-L24), the static bytecode has the same number of entries and exits, but the dynamic behavior may be different. I wonder which one the art guys claim to be enforcing at verification time (it seems like it would have to be the static bytecode, not the dynamic properties, but then they shouldn't be failing to verify this). looking at the google code issue, the comment https://code.google.com/p/android/issues/detail?id=80823#c6 was made by the same dev as https://code.google.com/p/android/issues/detail?id=80823#c3, so I sort of suspect there is some miscommunication going on. It is not clear in what context the dev is replying in, since in the previous comment you mention splitting monitor-enter and exit across methods. I think things would be much clearer if all patches, specialized clojure android builds, etc, were gotten rid of, then with a vanilla clojure jar you get a javap dump of what fails to verify, then just take that over to the android issue tracker and ask "hey, this fails to verify, why?"

Comment by Adam Clements [ 11/Dec/14 9:15 AM ]

Yeah, I shouldn't have confused it with the patched versions. The gist and the currently uploaded version use the vanilla clojure version of the locking macro now though.

I think the issue comes from the exception table and the instructions that covers. If line 24 can throw for example, you would end up at runtime with a monitor-exit, having never encountered a monitor-enter.

Comment by Ghadi Shayban [ 21/Dec/15 12:19 PM ]

According to Marcus Lagergren JRockit and Hotspot both account for lock releases being in a different method... Perhaps Android has a different (wrong) interpretation?

Comment by Alex Miller [ 21/Dec/15 3:33 PM ]

I added a new clj-1472.patch that fixes a few minor details I didn't like about the prior patch. However, it is still essentially the same change so I have retained the original author attribution.

Comment by Ghadi Shayban [ 21/Dec/15 4:14 PM ]

alex did you upload the right patch?

Comment by Alex Miller [ 21/Dec/15 4:58 PM ]

Ah, no messed that up. Will fix.

Comment by Nicola Mometto [ 22/Dec/15 3:54 AM ]

Alex, it's probably worth making that a `^:once fn*`

Comment by Ghadi Shayban [ 22/Dec/15 1:05 PM ]

From Android:

Android doesn't run Java bytecode, it runs Dex bytecode. The dexdump output of what your class translates to is interesting.

Neither is the JVMS interesting. Android isn't a Java Virtual Machine. We follow the JLS, but not the JVMS (how could we, when we don't run Java bytecode). As such, all appeals against it are irrelevant. We try to be compatible to the spirit of the JVMS wrt/ Dex bytecode, but if your source isn't Java, there are no guarantees.

Now, the verifiers were (and probably still are) broken, even against our (pretty bad) spec, and regrettably we aren't very consistent. In Marshmallow, for example, a lot of the code we can't correctly verify wrt/ structured locking is rejected as a VerifyError, which is not in the spirit of the JVMS. In the next release, this will be relaxed, however, and postponed to an actual check while running the code.

Regrettably there isn't anything we can do about old releases, you'll have to work around any issues. I'll try to take a look at your class when I find the time.

Sounds like making a workaround in Clojure is the least of all evils.

Comment by Alex Miller [ 22/Dec/15 1:46 PM ]

Added -2 patch with ^:once.

Comment by Alex Miller [ 12/Jan/16 10:30 AM ]

Our current belief is that Android is at fault here, so backlogging this for now.

Comment by Ghadi Shayban [ 13/Aug/18 2:58 PM ]

GraalVM/native-image is also complaining about monitorenter/exit balancing. Fixed it by mimicing what javac does: https://github.com/ghadishayban/clojure/commit/8acb995853761bc48b62190fe7005b70da692510

Comment by Alex Miller [ 13/Aug/18 3:57 PM ]

Ghadi, if that's a viable fix, I'm interested in a patch for that.

Comment by Ghadi Shayban [ 14/Aug/18 10:23 AM ]

If someone could assist me with Android testing infra, I'd be game.

In `javac`'s emission of synchronized, it installs a catch handler with the exception type "any". The linked commit catches Exception. If desired, we can emit the `any` handler (I don't know what that means yet exactly – Throwable + anything else in the future?) by calling GeneratorAdapter.visitTryCatch with `null` as the target class.

Comment by Adam Clements [ 14/Aug/18 10:56 AM ]

Have you tried applying the existing clj-1472-2.patch to see if that fixes GraalVM? I think we'd originally reached a place where people were happy with the attached patch (correct me if I'm wrong Alex) but it was decided that the JVM implementation is king so the problem was with Android for not conforming to that rather than with Clojure. It could be that decision is up for reconsideration if the same problem has reared its head elsewhere (I'd still like to see this patched myself).

This was an age ago now, but I remember trying all different combinations of clojure try/catch/finally/throw I could think of and not being able to get the emitted bytecode to conform to the spec for synchronised without altering clojure's code generation which was far too deep down to be practical - hence the above patch which relies on javac to generate the bytecode around the synchronisation instead of using monitor-enter and monitor-exit. I'd be concerned with your linked implementation that it might still generate wrong bytecode in some situations much like the existing implementation does even though it looks perfectly fine and apparently correct at the Clojure code level.

Comment by Ghadi Shayban [ 14/Aug/18 12:47 PM ]

I hadn't tried it but I'm sure clj-1472-2.patch would work from a feasability perspective. It has a perf downside: it can cause "profiling pollution" from the JVM's perspective and confuse lock optimization. (Most locks are uncontended)

I'd rather fix the emission to be more like javac, iff that works for Android.

Comment by Jason Felice [ 12/Dec/18 9:24 AM ]

I've tried the diff Ghadi linked to, but there are issues (including discarding the locked expression value). I fixed that, but after research, I've realized that:
1. This doesn't cover exceptions thrown by monitor-exit itself, which appears to be required. We can't recur in a catch, and we can't catch 'any', so we can't generate this bytecode with just Clojure constructs.
2. The bytecode emitted by Clojure's finally includes a store and a load uncovered by exception handling, so we can't just add exception handling inside the monitor-exit code. I'm pretty sure we can't remove the store and load.

So, unless I'm missing something, the only way to pass a structural locking check is to make locking a special form.

Comment by Adam Clements [ 12/Dec/18 11:10 AM ]

The already attached clj-1472-2.patch passes the structural locking tests.

After I did much exhaustive testing and comparing of generated bytecode back in 2014 this was the only viable approach I could find that didn't require instructing a new special form which reimplemented the poorly documented spec exactly to emit exactly the right bytecode, which seemed fragile to me.

I'd be interested to see some profiling numbers to see how big this claimed performance impact is, especially given how rare usage of the locking macro actually is in clojure (I could only find a few examples in the whole ecosystem when testing. Most people use the other concurrency primitives or java interop)

Comment by Alex Miller [ 12/Dec/18 12:27 PM ]

-3 patch rebases on master, no semantic changes, attribution retained

Comment by Jason Felice [ 13/Dec/18 4:11 PM ]

With clj-1472-3.patch, I can get builds working sometimes. Other times I get this error:

fatal error: com.oracle.svm.core.util.VMError$HostedError: Objects with relocatable pointers must be immutable              
        at com.oracle.svm.core.util.VMError.guarantee(VMError.java:85)                                                      
        at com.oracle.svm.hosted.image.NativeImageHeap.choosePartition(NativeImageHeap.java:492)                            
        at com.oracle.svm.hosted.image.NativeImageHeap.addObjectToBootImageHeap(NativeImageHeap.java:451)                   
        at com.oracle.svm.hosted.image.NativeImageHeap.addObject(NativeImageHeap.java:271)                                  
        at com.oracle.svm.hosted.image.NativeImageCodeCache.addConstantToHeap(NativeImageCodeCache.java:369)                
        at com.oracle.svm.hosted.image.NativeImageCodeCache.addConstantsToHeap(NativeImageCodeCache.java:356)               
        at com.oracle.svm.hosted.NativeImageGenerator.doRun(NativeImageGenerator.java:882)                                  
        at com.oracle.svm.hosted.NativeImageGenerator.lambda$run$0(NativeImageGenerator.java:401)                           
        at java.util.concurrent.ForkJoinTask$AdaptedRunnableAction.exec(ForkJoinTask.java:1386)                             
        at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)                                                  
        at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)                                      
        at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)                                              
        at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)

I'm on Graal rc10, and the shortest code I can find to generate that error is:

(ns rep.core
  (:gen-class))

(defn- foo [s]
  (locking s
    42))

(defn -main
  [& args]
  (foo))

Locking on something which isn't a parameter, like a var, works around the issue.

This seems like it might be a Graal issue, so I'll report it there.

Comment by Alex Miller [ 13/Dec/18 4:18 PM ]

Ok, please update. We’ve got probably a long while before the next release that might include this so good to get more feedback.

Comment by Nicola Mometto [ 14/Dec/18 10:17 AM ]

I'll vote against making `locking` into a new special form if possible. It would make any analysis-related library forward-incompatible and likely cause pains in dependency upgrade

Comment by Jason Felice [ 21/Jan/19 5:44 PM ]

With Graal VM rc11 (just released), clj-1472-3.patch works without issue.

Comment by JAre [ 14/Feb/19 9:49 PM ]

Started getting this error with GraalVM native compilation when migrated from Clojure 1.9 to 1.10
Minimal repro is:

(ns clj10-graal-repro.core
  (:require [clojure.spec.alpha :as s])
  (:gen-class))

(s/def ::foo (s/coll-of string?))

(defn -main
  [& args]
  (println (s/valid? ::foo ["bar"]))
  (println *clojure-version*))

I think the problem is that Clojure 1.10 depends on spec.alpha 0.2.176 that uses locking

More details in CLJ-2482





[CLJ-2481] [spec] every-impl breaks when passed records Created: 13/Feb/19  Updated: 13/Feb/19

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

Type: Defect Priority: Minor
Reporter: Sean Harrap Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

Mac OS X 10.13.6


Approval: Triaged

 Description   

Specs relying on every-impl (such as every, coll-of, etc) may break when passed a record. For example:

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

> (s/def ::coll-any (s/coll-of any?))
> (defrecord Pair [first second])

> (s/valid? ::coll-any 3)
false

> (s/valid? ::coll-any {:a 1 :b 2}
true

> (s/valid? ::coll-any (Pair. 1 2))
Execution error (UnsupportedOperationException) at user.Pair/empty (REPL:1).
Can't create empty: user.Pair





[CLJ-1509] Some clojure namespaces not AOT-compiled and included in the clojure jar Created: 20/Aug/14  Updated: 12/Feb/19

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

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: build

Attachments: Text File clj-1509.patch    
Patch: Code
Approval: Prescreened

 Description   

There is a list of namespaces to AOT in build.xml and several namespaces are missing from that list, thus no .class files for those namespaces are created or included in the standard clojure jar file as part of the build.

Missing namespaces include:

  • clojure.core.reducers
  • clojure.instant
  • clojure.parallel
  • clojure.uuid

Proposal: Attached patch adds clojure.instant and clojure.uuid to the compiled namespaces. clojure.parallel is deprecated and requires the JSR-166 jar so was not included.

Patch: clj-1509.patch



 Comments   
Comment by Alex Miller [ 20/Aug/14 1:06 PM ]

Looking at this a bit further, clojure.core.reducers uses the compile-if macro to determine what version of fork/join is available so AOT-compiling this namespace would fix that decision at build time rather than runtime, so it cannot be included.

Comment by Alex Miller [ 12/Feb/19 11:06 AM ]

As of Clojure 1.10, which relies on Java 1.8, all of these conditional build cases have been removed. I believe clojure.instant and clojure.uuid are being transitively compiled now, though not explicitly.

Comment by Alex Miller [ 12/Feb/19 11:17 AM ]

Actually clojure.parallel is still dependent on things that only exist in jsr166, but all the others have had conditionality removed. Added missing patch.





[CLJ-2480] Document that a promise can be invoked to deliver Created: 11/Feb/19  Updated: 11/Feb/19

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

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File clj-2480-2.patch     Text File promise-as-fn-docstring.patch    
Patch: Code
Approval: Prescreened

 Description   

Promises can be invoked to deliver a value to them (same result as calling deliver), but this is not mentioned in the docstring:

(def p (promise))
(p 42) ;; same as (deliver p 42)
@p ;; 42

Docstring before:

Returns a promise object that can be read with deref/@, and set,
  once only, with deliver. Calls to deref/@ ...

Proposed: Change docstring to:

clojure.core/promise
([])
  Returns a promise object that can be read with deref/@, and set,
  once only, with deliver or by invoking the promise. Calls to
  deref/@ ...

Patch: clj-2480-2.patch
Prescreened by: Alex Miller



 Comments   
Comment by Howard Lewis Ship [ 11/Feb/19 2:06 PM ]

I think we disagree about the value (or definition) of excessive concision in docstrings.





[CLJ-2444] Typo in docstring of test-vars Created: 23/Nov/18  Updated: 11/Feb/19

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

Type: Enhancement Priority: Trivial
Reporter: Michiel Borkent Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File CLJ-2444.patch    
Patch: Code
Approval: Screened

 Description   

Patch fixes docstring: "runs test-vars" should be "runs test-var".

Screened by: Alex Miller






[CLJ-2463] Improve error printing in clojure.main with -m, -e, etc Created: 04/Jan/19  Updated: 09/Feb/19

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: error-reporting, main

Attachments: Text File clj-2463.patch    
Approval: Vetted

 Description   

Clojure 1.10 has a lot of improvements to print better errors in clojure.main when using the repl. However, these improvements do not extend to the default behavior when using other clojure.main modes like -m (run -main in a namespace), -e (run expression), or the script running option. All of these will simply let an exception throw out of the main and default to JVM behavior, which prints the stack of the full chain of exceptions.

Some practical ramifications of this are that anything in the ecosystem that is running Clojure programs via clojure.main (could be clj, but could also be other tools like "lein ring server" where I saw this happenn), these tools are not getting the advantages of the error improvements.

The general proposal is to catch Throwable around the call to the main entry point and at least run the Throwable->map, ex-triage, ex-str pipeline. Also need to consider what to do about printing the stack trace. The default JVM behavior is to print the stack of every exception in the chain, but printing either the root cause stack (in execution error) or no stack (in reader/compiler/macroexpansion cases) would likely be better.

Simple repro (note very long line of spec problem data that all gets spammed to the screen - have to scroll right to see it):

$ clj -e "(ns foo (import java.util.Date))"
Exception in thread "main" Syntax error macroexpanding clojure.core/ns at (1:1).
Call to clojure.core/ns did not conform to spec.
	at clojure.lang.Compiler.checkSpecs(Compiler.java:6971)
	at clojure.lang.Compiler.macroexpand1(Compiler.java:6987)
	at clojure.lang.Compiler.macroexpand(Compiler.java:7074)
	at clojure.lang.Compiler.eval(Compiler.java:7160)
	at clojure.lang.Compiler.eval(Compiler.java:7131)
	at clojure.core$eval.invokeStatic(core.clj:3214)
	at clojure.main$eval_opt.invokeStatic(main.clj:465)
	at clojure.main$eval_opt.invoke(main.clj:459)
	at clojure.main$initialize.invokeStatic(main.clj:485)
	at clojure.main$null_opt.invokeStatic(main.clj:519)
	at clojure.main$null_opt.invoke(main.clj:516)
	at clojure.main$main.invokeStatic(main.clj:598)
	at clojure.main$main.doInvoke(main.clj:561)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.lang.Var.applyTo(Var.java:705)
	at clojure.main.main(main.java:37)
Caused by: clojure.lang.ExceptionInfo: Call to clojure.core/ns did not conform to spec. {:clojure.spec.alpha/problems [{:path [], :reason "Extra input", :pred (clojure.spec.alpha/cat :docstring (clojure.spec.alpha/? clojure.core/string?) :attr-map (clojure.spec.alpha/? clojure.core/map?) :ns-clauses :clojure.core.specs.alpha/ns-clauses), :val ((import java.util.Date)), :via [:clojure.core.specs.alpha/ns-form], :in [1]}], :clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__2509 0x412c995d "clojure.spec.alpha$regex_spec_impl$reify__2509@412c995d"], :clojure.spec.alpha/value (foo (import java.util.Date)), :clojure.spec.alpha/args (foo (import java.util.Date))}
	at clojure.spec.alpha$macroexpand_check.invokeStatic(alpha.clj:705)
	at clojure.spec.alpha$macroexpand_check.invoke(alpha.clj:697)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Var.applyTo(Var.java:705)
	at clojure.lang.Compiler.checkSpecs(Compiler.java:6969)
	... 15 more

As a macroexpansion spec error, this could just say:

$ clj -e "(ns foo (import java.util.Date))"
Syntax error macroexpanding clojure.core/ns at (1:1).
((import java.util.Date)) - failed: Extra input spec: :clojure.core.specs.alpha/ns-form

which would be much less scary.



 Comments   
Comment by Alex Miller [ 04/Jan/19 2:17 PM ]

Attached a proof-of-concept hack at this. Needs more discussion about what to do about the stack traces, whether we can do a better job sharing code between clojure.main and clojure.repl/pst, and maybe should consider an option (like -x) to dump the full stack chain in case you need it for debugging.

Comment by Stuart Halloway [ 09/Feb/19 7:16 AM ]

It is important to keep in mind all the ways that clojure.main is different from the REPL. When running the REPL, you still have the process after an exception happens. If you don't like the way exceptions are presented, you can programmatically grab the exception and do whatever your want. You can use the new 1.10 tools or write your own.

When clojure.main exits, your process is gone. If we make any change at all (particularly eliding detail for human reader convenience), and somebody wants something different, then they are out of luck. Also, printing the stack trace as Java does (the current behavior) means we are operationally compatible with production tooling designed around Java processes.

Most of the reported pain people feel from this would be fixed by making a tiny number of changes in how Java processes are launched from lein, boot, Cursive, et al. On the other side of such changes, these tools would have independent and full control of how users experience errors. They could match 1.10 behavior or enhance it, separate from the Clojure release cycle. And they could make different choices in different contexts.

Comment by Alex Miller [ 09/Feb/19 2:18 PM ]

Ideally, there should be both a programmatic entry point and a command line wrapper entry point that prints rather than leaves it to the jvm.





[CLJ-2476] Improve doc clarity regarding binding conveyance Created: 02/Feb/19  Updated: 07/Feb/19

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

Type: Enhancement Priority: Minor
Reporter: Matthias Varberg Ingesman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File clj-2476.patch    
Patch: Code
Approval: Prescreened

 Description   

It would be helpful for the binding conveyance feature of futures, agents and any other places that support it to be more prominent in the doc-strings. It could be as little as a note at the bottom saying something like: "This will propagate current thread's bindings to the new thread."

I have also added an issue to the [clojure-site github repo](https://github.com/clojure/clojure-site/issues/352) to add a note on binding conveyance to the vars reference page on clojure.org.

Patch: clj-2476.patch






[CLJ-2473] [core.specs] Destructuring spec is overly restrictive in namespaced :keys Created: 31/Jan/19  Updated: 31/Jan/19

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: core.specs, destructuring

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

 Description   

Namespaced :keys destructuring (see CLJ-1919) supports any kind of ident in a `::foo/keys` or `::foo/syms`, but the core spec says only `simple-symbol?` is allowed.

Example:

user=> (let [{::keys [:foo]} {::foo 1}] foo)
Syntax error macroexpanding clojure.core/let at (REPL:1:1).
...bunch of spec problems

Expected:

user=> (let [{::keys [:foo]} {::foo 1}] foo)
1

Proposed: Widen spec for this case from `simple-symbol?` to `ident?` (which the code supports).
Patch: clj-2473.patch



 Comments   
Comment by Thomas Heller [ 31/Jan/19 11:39 AM ]

ident? accepts namespaced keywords (and symbols) which probably should not be allowed given that the current implementation will ignore the namespace and use the namespace used by "keys" instead. To avoid confusion the spec should, in my opinion, be restricted to simple-keyword? or simple-symbol? in the namespaced "keys" case and only accept ident? for non-namespaced :keys.

(let [{:foo/keys [:other/bar]} {}])
(let [{:foo/keys [other/bar]} {}])
Comment by Alex Miller [ 31/Jan/19 12:14 PM ]

I considered that and decided to match the code. There is also consistency with :keys.





[CLJ-2472] [spec] 'check' silently ignores symbols which are not checkable Created: 30/Jan/19  Updated: 31/Jan/19

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

Type: Enhancement Priority: Minor
Reporter: Josip Gracin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting, spec
Environment:

[org.clojure/spec.alpha "0.2.176"], [org.clojure/core.specs.alpha "0.2.44"]


Attachments: Text File clj-2472-1.patch     Text File clj-2472-2.patch    
Approval: Triaged

 Description   

Function clojure.spec.test.alpha/check silently ignores symbols which are not "checkable". This seems counter-intuitive to me because I'd expect it to fail when I call check on a symbol and the symbol does not have a spec (e.g. it is misspelled).

The definition of check looks like this:

(defn check
 ([] (check (checkable-syms)))
 ([sym-or-syms] (check sym-or-syms nil))
 ([sym-or-syms opts]
    (->> (collectionize sym-or-syms)
         (filter (checkable-syms opts))
         (pmap
          #(check-1 (sym->check-map %) opts)))))

It seems to me that the (filter (checkable-syms opts)) might better go to the no-arg variant.



 Comments   
Comment by Josip Gracin [ 30/Jan/19 9:43 AM ]

Attaching clj-2472-1.patch. It preserves backward-compatibility which complicates it a bit.

Comment by Josip Gracin [ 31/Jan/19 3:24 AM ]

Minor change to the patch (using 'when' instead of 'if'). The idea of the solution is to introduce a new option :assert-checkable which asserts that provided syms are checkable.





[CLJ-2470] Unify behavior of shuffle in CLJ and CLJS Created: 23/Jan/19  Updated: 29/Jan/19

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

Type: Enhancement Priority: Minor
Reporter: Michiel Borkent Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJ-2470-2.patch     Text File CLJ-2470.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

The CLJS version of shuffle accepts arrays, but the CLJ version does not.
The CLJS version of shuffle accepts nil, but the CLJ version does not.

This behavior could be unified.

Proposal:

  • Add support for arrays in CLJ
  • Add support for nil in CLJ and return nil or ().
  • The behavior of the CLJS version on nil should be made the same as CLJ: do not return an empty vector, since nil does not pun as an empty vector.
  • The CLJS version supports a shuffle of string, but this can be considered undefined behavior.

The attached patch CLJ-2470 adds support for nil (also returns nil) and Java arrays.
Patch CLJ-2470-2 adds a better error message.

Patch: CLJ-2470-2.patch
Prescreened by: Alex Miller



 Comments   
Comment by Michiel Borkent [ 25/Jan/19 11:35 AM ]

Patch attached.

Comment by Michiel Borkent [ 28/Jan/19 11:05 PM ]

Alex:

CLJS also supports:

(shuffle (eduction (map inc) [1 2 3]))

Should it and if so, should we support it in CLJ? Probably not, since it's not a collection?

Comment by Alex Miller [ 29/Jan/19 12:09 PM ]

That seems weird to me, so I'd say no.





[CLJ-2469] pr botches structmaps containing namespaced keys when *print-namespace-maps* Created: 20/Jan/19  Updated: 20/Jan/19

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

Type: Defect Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print, regression

Approval: Triaged

 Description   

pr misrepresents a structmap containing namespaced-keyword keys. pr misstates actual keys' values, and includes phantom keys.

user=> (pr-str (struct (create-struct :q/a :q/b :q/c) 1 2 3))
"#:q{:q/a nil, :q/b nil, :q/c nil, :c 3, :b 2, :a 1}"

It worked in Clojure 1.8, and disabling the print-namespace-maps feature appears to work around the problem:

user=> (binding [*print-namespace-maps* false] 
         (pr-str (struct (create-struct :q/a :q/b :q/c) 1 2 3)))
"{:q/a 1, :q/b 2, :q/c 3}"

By the way: Structmaps are (now as ever) appropriate when the basis is anonymous and computed (not coded) or key order is a significant aspect of the basis. People sometimes cite the reference doc's statement, "Most uses of StructMaps would now be better served by records", but, of course, it is referring only to those uses that are actually better served by records. Record classes can't be anonymous and defrecord needs the basis at compile time; and normal maps do not exhibit a preset key order. Thus the continuing relevance of structmaps.






[CLJ-1941] [spec] Instrumentation of fns with primitive type hints fails Created: 01/Jun/16  Updated: 18/Jan/19

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

Type: Defect Priority: Minor
Reporter: Kenny Williams Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: spec
Environment:

Ubuntu 15.10
Using boot 2.6.0 on openjdk version "1.8.0_91"


Approval: Vetted

 Description   
(require '[clojure.spec :as s] '[clojure.spec.test :as st])
(defn foo [^double val] val)
(s/fdef foo :args (s/cat :val double?))
(st/instrument `foo)
(foo 5.2)

user=> (foo 5.2)
ClassCastException clojure.spec.test$spec_checking_fn$fn__13069 cannot be cast to clojure.lang.IFn$DO
       	user/eval6 (NO_SOURCE_FILE:5)
       	user/eval6 (NO_SOURCE_FILE:5)
       	clojure.lang.Compiler.eval (Compiler.java:6951)
       	clojure.lang.Compiler.eval (Compiler.java:6914)
       	clojure.core/eval (core.clj:3187)
       	clojure.core/eval (core.clj:3183)
       	clojure.main/repl/read-eval-print--9704/fn--9707 (main.clj:241)
       	clojure.main/repl/read-eval-print--9704 (main.clj:241)
       	clojure.main/repl/fn--9713 (main.clj:259)
       	clojure.main/repl (main.clj:259)
       	clojure.main/repl-opt (main.clj:323)
       	clojure.main/main (main.clj:422)

Cause: spec replaces var values with instrumented functions that will not work with primitive function interfaces

Approach: Take primitive interfaces into account and make them work, or document/fail that instrumentation will not work with these.



 Comments   
Comment by Kevin Downey [ 02/Jun/16 1:41 AM ]

spec replaces var values with instrumented functions, which works for the default linking case, var deref cast to ifn, invoke, but in the other cases (primitive functions, direct linking, others?) this won't work

Comment by Kenny Williams [ 02/Jun/16 3:39 PM ]

Hmm. Well this should be at least be documented. So, spec cannot be used on functions with a type hinted arg?

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

Spec cannot be used on functions with primitive typed hinted arguments or returns – non-primitive type hints seem to be fine.

But documentation isn't enough here: instrumenting a namespace and then discovering it broke a function (that happened to have a primitive type hint) isn't acceptable. If the instrumentation isn't going to work, the function should be skipped (and a warning produced, hopefully).

Comment by Kevin Downey [ 02/Jun/16 8:10 PM ]

yeah, I was giving the root cause of the issue, not excusing the issue.

Understanding the root cause predicts other places where there will be issues: where ever some non-default function linking strategy is used.

One such place is direct linked functions, but I suspect for direct linked functions, Clojure/Core will just say you should only instrument code for testing, and you should only turn on direct liking for production.

Another case, which I am sort of surprised we haven't heard more about yet is protocol functions.

Comment by Sean Corfield [ 02/Jun/16 8:35 PM ]

Your comment about direct linking made me wonder about the validity of spec'ing and instrumenting clojure.core functions. The examples show clojure.core/symbol, but Clojure's core library is shipped as direct linked, as of 1.8.0 isn't it?

Comment by Kevin Downey [ 03/Jun/16 3:14 AM ]

what alters the calling convention isn't the function being compiled with direct linking on, but a caller of that function being compiled with direct linking on.

This code will throw a non-conforming error for the bogus symbol spec with direct linking off, and return the symbol foo with direct linking on

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

(s/fdef symbol
  :args string?
  :ret symbol?)

(defn foo
  []
  (symbol 'foo))

(s/instrument-all)

(foo)
Comment by Kevin Downey [ 03/Jun/16 3:26 AM ]

This code returns true because m is a protocol function, if you replace it with a regular function it throws a non-conforming error

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

(defprotocol P
  (m [_]))

(deftype T []
  P
  (m [_] true))

(s/fdef m
  :args (s/cat :p (constantly false))
  :ret string?)

(defn foo
  []
  (m (T.)))

(s/instrument-all)

(foo)
Comment by Alex Miller [ 13/Jun/16 3:53 PM ]

@Sean instrumenting core functions will work for calls from your code into core (which are presumably not direct-linked), but will not affect calls from one core function to another as they are direct-linked and do not go through the var. One thing we've considered for a long while is building a dev version of core that would not be direct-linked and could potentially turn on instrumentation or other helpful dev-time tooling.

Comment by Sean Corfield [ 14/Jun/16 5:48 PM ]

Thanks for that answer @alexmiller – We have dev set to non-direct-linking but QA / production set to direct linking, so I'm only concerned about possible issues in dev with (s/instrumental-all) and wanting to be sure "code won't break". If instrumentation won't affect existing (direct-linked) calls within core, that's good enough for me. I am concerned about primitive hinting and protocols (and whatever crawls out of the woodwork here) since you don't want to be forced to read the source of every library you include, just to see whether (s/instrument-all) is safe or whether it will bite you in some weird way while you're developing.

Comment by Michiel Borkent [ 18/Jan/19 3:24 AM ]

Running into this issue while spec'ing clojure.core/partition-all.





[CLJ-1496] Added a new arity to 'ex-info' that only accepts a message. Created: 08/Aug/14  Updated: 17/Jan/19

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

Type: Feature Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 4
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    
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.

Comment by Jan Krajicek [ 17/Jan/19 5:30 AM ]

This would be a nice way to throw simple exceptions from cljc files that works in both clj and cljs.





[CLJ-2466] java.time.ZonedDateTime is not an inst Created: 10/Jan/19  Updated: 16/Jan/19

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

Type: Enhancement Priority: Minor
Reporter: The Alchemist Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Patch: Code

 Description   

Description

(inst? (java.time.ZonedDateTime/now))
=> false

Seems incorrect, as ZonedDateTime has the necessary information.

Proposed Solution

Conversion between Instant and ZonedDateTime is lossless.

(extend-protocol clojure.core/Inst
  java.time.ZonedDateTime
  (inst-ms* [inst] (.toEpochMilli (.toInstant ^java.time.ZonedDateTime inst))))





[CLJ-2467] ^:const no-field record reports generic class Created: 15/Jan/19  Updated: 16/Jan/19

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

Type: Defect Priority: Minor
Reporter: Maarten Truyens Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Approval: Triaged

 Description   

Example:

(defrecord ABC [some-field])
(def ^:const abc (->ABC :some-value))
(println (class abc))

(defrecord XYZ [])
(def ^:const xyz (->XYZ))
(println (class xyz))

results in

"sx.clj.temp.ABC
clojure.lang.PersistentArrayMap"

The XYZ seems to lose its class information, and is apparently stored as a simple map, instead of a record. I could not find the official specification of ^:const, but this behaviour seems incorrect to me, or at least inconsistent.



 Comments   
Comment by Alex Miller [ 16/Jan/19 11:20 PM ]

Possibly related: CLJ-1093, CLJ-1575, CLJ-1460





[CLJ-2450] [spec] defining new multimethod for a speced multi method fails under instrumentation Created: 06/Dec/18  Updated: 08/Jan/19

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

Type: Defect Priority: Major
Reporter: Erik Assum Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec

Approval: Triaged

 Description   
23:11 $ clj -Sdeps '{:deps {org.clojure/clojure {:mvn/version "1.10.0-RC3"} org.clojure/test.check {:mvn/version "0.10.0-alpha2"}}}'
Clojure 1.10.0-RC3
user=> (require '[clojure.spec.alpha :as s])
nil
user=> (require '[clojure.spec.test.alpha :as st])
nil
user=> (defmulti foo identity)
#'user/foo
user=> (s/fdef foo :args (s/cat :wat string?))
user/foo
user=> (defmethod foo :bar [lol])
#object[clojure.lang.MultiFn 0x57ac5227 "clojure.lang.MultiFn@57ac5227"]
user=> (st/instrument)
[user/foo]
user=> (defmethod foo :baz [lol])
Execution error (ClassCastException) at user/eval150 (REPL:1).
clojure.spec.test.alpha$spec_checking_fn$fn__3026 cannot be cast to clojure.lang.MultiFn
user=> *e
#error {
 :cause "clojure.spec.test.alpha$spec_checking_fn$fn__3026 cannot be cast to clojure.lang.MultiFn"
 :via
 [{:type java.lang.ClassCastException
   :message "clojure.spec.test.alpha$spec_checking_fn$fn__3026 cannot be cast to clojure.lang.MultiFn"
   :at [user$eval150 invokeStatic "NO_SOURCE_FILE" 1]}]
 :trace
 [[user$eval150 invokeStatic "NO_SOURCE_FILE" 1]
  [user$eval150 invoke "NO_SOURCE_FILE" 1]
  [clojure.lang.Compiler eval "Compiler.java" 7176]
  [clojure.lang.Compiler eval "Compiler.java" 7131]
  [clojure.core$eval invokeStatic "core.clj" 3214]
  [clojure.core$eval invoke "core.clj" 3210]
  [clojure.main$repl$read_eval_print__9068$fn__9071 invoke "main.clj" 414]
  [clojure.main$repl$read_eval_print__9068 invoke "main.clj" 414]
  [clojure.main$repl$fn__9077 invoke "main.clj" 435]
  [clojure.main$repl invokeStatic "main.clj" 435]
  [clojure.main$repl_opt invokeStatic "main.clj" 499]
  [clojure.main$main invokeStatic "main.clj" 598]
  [clojure.main$main doInvoke "main.clj" 561]
  [clojure.lang.RestFn invoke "RestFn.java" 397]
  [clojure.lang.AFn applyToHelper "AFn.java" 152]
  [clojure.lang.RestFn applyTo "RestFn.java" 132]
  [clojure.lang.Var applyTo "Var.java" 705]
  [clojure.main main "main.java" 37]]}
user=>

I would expect this to not throw an exception



 Comments   
Comment by Dennis Schridde [ 08/Jan/19 10:24 AM ]

For reference and in case someone else would also like to know the call order that works (with Clojure 1.10.0 and clojure.spec.alpha 0.2.176):

Unable to find source-code formatter for language: clj. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(require '[clojure.spec.alpha :as s])
; => nil
(require '[clojure.spec.test.alpha :as stest])
; => nil
(defmulti testfn :type)
; => #'user/testfn
(defmethod testfn :atype [m] 1)
; => #object[clojure.lang.MultiFn 0x5d16b612 "clojure.lang.MultiFn@5d16b612"]
(s/fdef testfn :args (s/cat :m (s/keys :req-un [::type ::does-not-exist])))
; => user/testfn
(stest/instrument `testfn)
; => [user/testfn]
(testfn {:type :atype :does-not-exist nil})
; => 1
(testfn {:type :atype})
; => clojure.lang.ExceptionInfo: Call to #'user/testfn did not conform to spec.




[CLJ-2462] gen-class, gen-interface docstrings unclear about "when compiling..." Created: 02/Jan/19  Updated: 03/Jan/19

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring


 Description   

gen-class and gen-interface docstrings begin, "When compiling,...". It is not clear what that means.

After all, when is Clojure not compiling? The clojure.org home page says "Clojure is a compiled language" and the docstrings for unchecked-math, warn-on-reflection, case, and defmacro suggest that compiling is turning forms into bytecode - a nonstop phenomenon.

Elsewhere in clojure core: The defrecord and deftype docstrings refer to "AOT compiling". The compile-files docstring refers to "compiling files", not clear if referring to input files or output files. The docstring of "compile" refers to "classfiles". These may or may not all be related.

If gen-class, gen-interface, compile, and compile-files mean "AOT compiling", then let's make that explicit. If they don't, then let's please make their true meaning clear.



 Comments   
Comment by Alex Miller [ 03/Jan/19 8:14 AM ]

It means "AOT compiling", ie writing class files to disk - *compile-files* is bound to true.





[CLJ-2343] define and load classes in memory with gen-class Created: 30/Mar/18  Updated: 02/Jan/19

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: gen-class

Attachments: Text File 0001-CLJ-2343-define-and-load-class-while-JITing-gen-clas.patch    
Patch: Code
Approval: Triaged

 Description   

Currently gen-class only works while AOT compiling, but just evaluates to nil while JIT loading.
The reason for this behaviour is historical and no longer relvant since CLJ-979 landed in 1.7, which fixed the dynamic classloader definition issues and also made this exact same change for gen-interface. The only reason why this wasn't also done for gen-class is that I forgot about it.

This patch fixes this inconsistency

Patch: 0001-CLJ-2343-define-and-load-class-while-JITing-gen-clas.patch



 Comments   
Comment by Kari Marttila [ 01/Jan/19 12:33 PM ]

Just asking if the issue I asked in Clojure Slack relates to this JIRA issue.
If I have a gen-class (e.g. mygenclass) as part of my Clojure project and I want to refresh all namespaces in Clojure REPL using command:
(do (require '[clojure.tools.namespace.repl :refer [refresh]]) (refresh))
... then I get an error: "namespace 'mygenclass' not found after loading 'mygenclass'.

Comment by Kimmo Koskinen [ 02/Jan/19 3:37 PM ]

Now that there is Deps and CLI, this change would make it even more easier package a library using that uses gen-class, since no class files would need to be packaged (if I understood correctly).





[CLJ-1966] [spec] :clojure.spec/invalid is not a valid :clojure.spec/any value Created: 21/Jun/16  Updated: 02/Jan/19

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

Type: Defect Priority: Minor
Reporter: Alexander Kiel Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: spec


 Description   

(clojure.spec/valid? :clojure.spec/any :clojure.spec/invalid) returns false

This issue gets serious, if one likes to write specs for core functions like = which are used by spec itself. I observed this bug as I wrote a spec for assoc.

A possible solution could be to use an (Object.) sentinel internally and :clojure.spec/invalid only at the API boundary. But I have not thought deeply about this.



 Comments   
Comment by Alexander Kiel [ 24/Jun/16 9:48 AM ]

I have another example were the described issue arises. It's not possible to test the return value of a predicate suitable for conformer, because it should return :clojure.spec/invalid itself.

(ns coerce
  (:require [clojure.spec :as s]))

(s/fdef parse-long
  :args (s/cat :s (s/nilable string?))
  :ret (s/or :val int? :err #{::s/invalid}))

(defn parse-long [s]
  (try
    (Long/parseLong s)
    (catch Exception _
      ::s/invalid)))
Comment by Alexander Kiel [ 12/Jul/16 10:01 AM ]

No change in alpha 10 with the removal of :clojure.spec/any and introduction of any?.

Comment by Sean Corfield [ 12/Sep/16 4:06 PM ]

Another example from Slack, related to this:

(if-let [a 1]
  ::s/invalid)

Fails compilation (macroexpansion) because ::s/invalid causes the spec for if-let to think the then form is non-conforming.

Workaround:

(if-let [a 1]
  '::s/invalid)
Comment by Ambrose Bonnaire-Sergeant [ 05/Sep/17 3:41 PM ]

Another example from the wild: https://github.com/pjstadig/humane-test-output/pull/23

A macro rewriting

(is (= ::s/invalid ..))

to

(let [a ::s/invalid] ...)

resulted in some very strange errors.

Comment by Alexander Kiel [ 13/Sep/17 6:34 AM ]

The macro issues can be solved by just not using ::s/invalid in code directly. I think in general, it better to use the predicate s/invalid?.

Instead of writing:

(= ::s/invalid ...)

one should use

(s/invalid? ...)

But I have no idea to solve the issue where you have ::s/invalid in data which is validated. The function spec for identical? is a good example.

(s/fdef clojure.core/identical?
  :args (s/cat :x any? :y any?)
  :ret boolean?)

world not work.

Comment by jcr [ 06/Jul/18 9:05 AM ]

Please use sumtypes instead of "magic" values to indicate failure or success. For example,

(s/conform any? ::s/invalid) ;=> [:ok ::s/invalid]
(s/conform int? ::s/invalid) ;=> [:failure #::s{:problems ... :spec ... :value ...}]

Note that the return value should be an instance of clojure.lang.MapEntry, in order for key and val to work on it. However, if it's not desirable to return the explain-map on failure then returning vectors [:ok value] and [:failure] (no second element) would work as well.

Since spec is explicitly in alpha, it's not too late yet to fix the api.

Related: CLJ-2115

Comment by Michiel Borkent [ 30/Dec/18 8:40 AM ]

This is also a problem in tooling REPL like Cursive's. Repro:

(ns assoc.core
  (:require
   [clojure.spec.alpha :as s]
   [clojure.spec.test.alpha :as stest]))

(s/fdef clojure.core/assoc
  :args (s/cat :map (s/nilable associative?)
               :key any? :val any? :kvs (s/* (s/cat :ks any? :vs any?)))
  :ret associative?)

(stest/instrument `assoc)

(s/conform string? 1)

Evaluating this namespace in a Cursive REPL gives:

Error printing return value (ExceptionInfo) at clojure.spec.test.alpha/spec-checking-fn$conform! (alpha.clj:132).
Call to #'clojure.core/assoc did not conform to spec.

The problem can also reproduced without Cursive by evaluating the above and this:

(defn tooling-repl
  ([] (tooling-repl {}))
  ([state]
   (let [l (read-line)
         evaled (eval (read-string l))]
     (prn evaled)
     (recur (assoc state
                   :*1 evaled
                   :*2 (get state :*1))))))

;; now type in the REPL: (s/conform string? 1)
(with-redefs [read-line (constantly "(s/conform string? 1)")]
  (tooling-repl))
Comment by Michiel Borkent [ 30/Dec/18 4:35 PM ]

I modified the ::any spec in speculative as follows:

(s/def ::any
  (s/with-gen
    (s/conformer #(if (s/invalid? %) ::invalid %))
    #(s/gen any?)))




[CLJ-2453] Allow reader conditionals in prepl Created: 10/Dec/18  Updated: 02/Jan/19

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

Type: Enhancement Priority: Minor
Reporter: Oliver Caldwell Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: prepl

Attachments: Text File prepl-reader-conditionals.patch    
Patch: Code
Approval: Vetted

 Description   

The ClojureScript prepl implementation allows for reader conditionals, the Clojure one does not. Allowing reader conditionals in the Clojure prepl implementation will bring the two implementations in line with each other.



 Comments   
Comment by Oliver Caldwell [ 19/Dec/18 3:39 AM ]

Enables reader conditionals in Clojure prepl.





[CLJ-2461] [core.specs] refer-clojure and import specs are too limited in quote support Created: 30/Dec/18  Updated: 30/Dec/18

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

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: core.specs

Approval: Triaged

 Description   

The refer-clojure spec added in CLJ-2062 supports only limited quoting.

In Clojure 1.8, these are both allowed:

user=> (in-ns 'myns)
#object[clojure.lang.Namespace 0x579d011c "myns"]
myns=> (clojure.core/refer-clojure :only '[+ -])
nil
myns=> (clojure.core/refer-clojure :only ['+ '-])
nil

But in 1.9/1.10, the first is valid but the second will throw:

Clojure 1.10.0
user=> (in-ns 'myns)
#object[clojure.lang.Namespace 0x61e45f87 "myns"]
myns=> (clojure.core/refer-clojure :only '[+ -])
nil
myns=> (clojure.core/refer-clojure :only ['+ '-])
Syntax error macroexpanding clojure.core/refer-clojure at (REPL:1:1).
(quote +) - failed: #{(quote quote)} at: [:only :arg :quoted-spec :quote]
(quote +) - failed: simple-symbol? at: [:only :arg :spec] spec: :clojure.core.specs.alpha/only
(quote -) - failed: simple-symbol? at: [:only :arg :spec] spec: :clojure.core.specs.alpha/only

Cause: This is due to the way the quoted spec is defined, allowing only a quoted list rather than a quoted list of symbols or a list of quoted symbols. A similar issue is seen with import - (import ['java.util 'Date]) fails in a similar way, but it's uncommon for people to invoke import by quoting the internal symbols rather than the outer list (import '[java.util Date]).

Proposed: To match support in 1.8, would need specs that include both quoted lists of symbols and lists of quoted symbols.



 Comments   
Comment by Alex Miller [ 30/Dec/18 8:57 AM ]

Due to the rarity of this usage pattern (this is the first report in over 2 years since this was added), I'm going to mark this minor priority.





[CLJ-2459] ExceptionInInitializerError if jars executed with java -jar Created: 20/Dec/18  Updated: 21/Dec/18

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

Type: Defect Priority: Trivial
Reporter: Piotr Zygielo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: build
Environment:

N/A


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

 Description   

Main-Class set in manifests of target/clojure.jar (or jars in Maven repository) gives impression, that they can be run as standalone JAR-packaged applications.

However running them with

java -jar clojure-${version}.jar
results in

Exception in thread "main" java.lang.ExceptionInInitializerError
    at clojure.main.<clinit>(main.java:20)
    Caused by: Syntax error compiling at (clojure/main.clj:1:1).
    at clojure.lang.Compiler.load(Compiler.java:7647)
    ...
    ... 1 more
    Caused by: java.io.FileNotFoundException: Could not locate clojure/spec/alpha__init.class, clojure/spec/alpha.clj or clojure/spec/alpha.cljc on classpath.
    at clojure.lang.RT.load(RT.java:466)
    at clojure.lang.RT.load(RT.java:428)

(It of course works perfectly fine for top-level clojure.jar built in local profile with dependencies shaded.)

Yet, for java -jar classpath cannot be modified in command line (-cp is ignored), only as Class-Path entries in manifest.
I'd prefer to see clean

no main manifest attribute...
reported by java rather than ExIIError and stack trace in such case.

Screened by: Alex Miller - correctly builds normal Clojure without a main class, but continues to build local jar with a main (that one includes the spec deps).



 Comments   
Comment by Alex Miller [ 21/Dec/18 8:25 AM ]

Thanks, good call.





[CLJ-1473] Badly formed pre/post conditions silently passed Created: 24/Jul/14  Updated: 19/Dec/18

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: 8
Labels: errormsgs

Attachments: Text File 0001-Validate-that-pre-and-post-conditions-are-vectors.patch     Text File CLJ-1473_v02.patch     Text File CLJ-1473_v03.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Before:

user=> ((fn [x] {:pre (pos? x)} x) -5) ; ouch!
-5
user=> ((fn [x] {:pre [(pos? x)]} x) -5) ; meant this
AssertionError Assert failed: (pos? x)  user/eval4075/fn--4076 (form-init5464179453862723045.clj:1)

After:

user=> ((fn [x] {:pre (pos? x)} x) -5)
CompilerException java.lang.IllegalArgumentException: Pre and post conditions should be vectors, compiling:(NO_SOURCE_PATH:1:2) 
user=> ((fn [x] {:pre [(pos? x)]} x) -5)                                  
AssertionError Assert failed: (pos? x)  user/eval2/fn--3 (NO_SOURCE_FILE:2)
user=> ((fn [x] {:post (pos? x)} x) -5)
CompilerException java.lang.IllegalArgumentException: Pre and post conditions should be vectors, compiling:(NO_SOURCE_PATH:3:2) 
user=> ((fn [x] {:post [(pos? x)]} x) -5)              
AssertionError Assert failed: (pos? x)  user/eval7/fn--8 (NO_SOURCE_FILE:4)

Patch: CLJ-1473_v03.patch
Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 29/Apr/15 1:54 PM ]

Would be nice to include the bad condition in the error (maybe via ex-info?) and also have tests.

Comment by Brandon Bloom [ 03/May/15 12:11 PM ]

New patch includes tests. Unfortunately, can't call ex-info directly due to bootstrapping concerns. Instead, just calls ExceptionInfo constructor directly.

Comment by Alex Miller [ 04/May/15 9:41 AM ]

Bug in the reporting: {:post pre} should be {:post post}.

Test should be improved as it could have caught that.

Comment by Brandon Bloom [ 04/May/15 7:25 PM ]

Good catch with the pre/post copy/paste screw up. Didn't enhance the test though, since that would involve creating an ex-info friendly variant of fails-with-cause

Comment by Rich Hickey [ 09/Oct/15 7:32 AM ]

:pre and :post don't require vectors, just collections

Comment by Andy Fingerhut [ 15/Nov/15 2:39 PM ]

Eastwood 0.2.2, released on Nov 15 2015, will warn about several kinds of incorrect pre and postconditions. See https://github.com/jonase/eastwood#wrong-pre-post

The Eastwood documentation may be misleading right now, in that it says that :pre and :post should be vectors, which is at odds with Rich's comment of Oct 9 2015. Corrections to Eastwood's documentation here are welcome. I guess Rich's intent is that :pre and :post could be vectors, lists, or sets? Would a map ever make sense there?

Comment by Marco Molteni [ 31/Jan/18 10:58 AM ]

Hello any news ?

Comment by Alex Miller [ 02/Aug/18 2:49 PM ]

There's feedback above from Rich that has never been addressed here. I took a stab at an update, but relaxing the constraint to just "collections of expressions" is kind of interesting. Once you do that, then the first example in the ticket here {:pre (pos? x)} is actually not invalid, it's a collection of two things: pos? and x. pos? evaluates to true (it's a function) and x evaluates to true for any number.

I'm not sure what the way forward is on that. It's perfectly valid and even potentially useful to refer to an incoming arg as a precondition, which would just be a symbol:

( (fn [v] {:pre [v]} v) nil)  ;; precondition should fail
( (fn [v] {:pre (v)} v) nil)  ;; equally valid

Maybe you could check for whether the elements of pre or post are either lists (expressions) or symbols that are arg bindings? Anything else is vacuously true and probably a bug. Not sure.

Comment by Andy Fingerhut [ 03/Aug/18 4:05 AM ]

Eastwood still gives warnings about code like the following:

(defn foo [x]
  {:pre [pos? x]}
  (inc x))

Here is sample output:

src/filename.clj:5:22: wrong-pre-post: Precondition found that is probably always logical true or always logical false.  Should be changed to function call?  pos?

It also still warns if the value of :pre or :post is not a vector, despite Rich's comment, so it is overly strict in that sense, but maybe not so bad to be overly strict there.

Comment by Martin Klepsch [ 19/Dec/18 8:29 AM ]

Just want to point to this PR (https://github.com/pedestal/pedestal/pull/544) as an example of how this affects production Clojure code.

Due to malformed pre/post conditions silently being accepted the API was essentially different from what the library authors intended. People started to rely on this and fixing the broken precondition essentially resulted in a breaking change.





[CLJ-2383] Add classes that were introduced to java.lang in Java 7/8 into DEFAULT_IMPORTS Created: 08/Aug/18  Updated: 18/Dec/18

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

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

Attachments: Text File clj-2383.patch    
Patch: Code
Approval: Prescreened

 Description   

Now that Clojure 1.10 alpha-6 dropped support for Java <=7, it seems reasonable to add the classes that were newly introduced to the java.lang package in Java <=8, into Clojure's DEFAULT_IMPORTS.

Here is the list of those classes I'm aware of:

Introduced in Java 7:

  • AutoCloseable
  • ClassValue
  • ReflectiveOperationException
  • BootstrapMethodError
  • SafeVarargs

Introduced in Java 8:

  • FunctionalInterface

Prescreened by: Alex Miller



 Comments   
Comment by Ghadi Shayban [ 08/Aug/18 10:18 AM ]

Besides AutoCloseable, the rest of those classes are rarely used. I'd rather import them explicitly over polluting the ns imports tbh. (Interfaces annotated with FunctionalInterface, an annotation type, are common; using it explicitly isn't)

There is a minor startup time cost to additional classloading, for classes not already loaded by the java.base module. I've experimented with dropping a bunch of rarely used default imports (a breaking change, no doubt).

Comment by Alex Miller [ 08/Aug/18 11:23 AM ]

I think we should uphold what has been true and stated in the past - stuff in java.lang is auto-imported. I do not think autoimporting 6 classes will affect startup time.

Comment by Shogo Ohta [ 18/Dec/18 4:46 AM ]

Added the patch, just in case





[CLJ-2303] Reified object doesn't satisfy protocol at compile time Created: 29/Dec/17  Updated: 17/Dec/18

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

Type: Defect Priority: Major
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

https://github.com/MichaelBlume/reify-fail

(defprotocol Foo
  (hello [this] "Says hello"))

(def bar
  (reify Foo
    (hello [this] "hello, world")))

(when-not (satisfies? Foo bar)
  (throw (Exception. "bar doesn't satisfy Foo")))

Notes:

Project is AOT
Another namespace requires this namespace. That namespace is compiled first. Without that namespace the problem goes away.



 Comments   
Comment by Kevin Downey [ 01/Jan/18 5:15 PM ]

what steps do you take after checking out the repo to reproduce this issue?

I cloned the repo and ran `lein compile` and didn't get any errors.

Comment by Michael Blume [ 01/Jan/18 10:56 PM ]

I was reproducing the issue with `lein check`, but `lein compile` also fails for me, so I'm confused.

Comment by Michael Blume [ 01/Jan/18 10:56 PM ]

I think it's important that the near-empty namespace compiles first, maybe there are platform differences in compilation order?

Comment by Michael Blume [ 01/Jan/18 10:57 PM ]

test repo fails for me under Mac OS and Ubuntu, both with Java 8

Comment by Michael Blume [ 01/Jan/18 10:58 PM ]

Under Ubuntu I have

$ lein -v
Leiningen 2.7.1 on Java 1.8.0_151 OpenJDK 64-Bit Server VM

Comment by Andy Fingerhut [ 02/Jan/18 12:19 AM ]

On both of these OS’s: Ubuntu 16.04.3 Linux, macOS 10.12.6

With both of these versions of Leiningen: 2.7.1, 2.8.1

I believe all of my experiments were using a recent version of JDK 1.8

With any of these Clojure versions: 1.7.0 1.8.0 1.9.0

I get an error with both the commands 'lein check' and 'lein compile' with the project https://github.com/MichaelBlume/reify-fail, but only if I do '/bin/rm -fr target' first (or run it before the target directory has ever been created, e.g. immediately after running the ‘git clone’ command for the repository).

If I run the command twice, the first command creates many .class files in the target directory, and the second such command gives no error.

Comment by Kevin Downey [ 02/Jan/18 12:36 AM ]

I am able to reproduce there error if I replace `:aot :all` with `:aot [com.lendup.citadel.pipeline com.lendup.citadel.providers.mocky]`. I suspect lein could fix this by doing their stale ns check between compiling namespaces to avoid repeated compilation of transitive namespaces.

Comment by Kevin Downey [ 02/Jan/18 12:41 AM ]

could be related to https://github.com/technomancy/leiningen/issues/2316

Comment by Tommi Reiman [ 16/Dec/18 10:23 AM ]

related: https://github.com/technomancy/leiningen/issues/2508.

Comment by Nicola Mometto [ 17/Dec/18 8:29 AM ]

Is this a clojure or Lein issue then? Can we reproduce this using bare `clj`?

Comment by Alex Miller [ 17/Dec/18 8:47 AM ]

I suspect you could reproduce it by compiling things in a certain order, but I have not tried to do so. But the resulting answer might be: compile things in the right order.





[CLJ-2457] Document return value of Throwable->map Created: 17/Dec/18  Updated: 17/Dec/18

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

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

Approval: Vetted

 Description   

The docstring for Throwable->map does not explain any of the details of what is returned.

The shape of the map should be described: What keys are always present? Which keys are optional, and when will optional keys be present? What are the values?

A spec would also be helpful.






[CLJ-2456] Clojure-aware error stacktrace rendering Created: 15/Dec/18  Updated: 15/Dec/18

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

Type: Enhancement Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Approval: Vetted

 Description   

Currently (1.10.0-RC5) stacktraces look like this:

user=> (Exception.)
#error {
 :cause nil
 :via
 [{:type java.lang.Exception
   :message nil
   :at [user$eval3 invokeStatic "NO_SOURCE_FILE" 1]}]
 :trace
 [[user$eval3 invokeStatic "NO_SOURCE_FILE" 1]
  [user$eval3 invoke "NO_SOURCE_FILE" 1]
  [clojure.lang.Compiler eval "Compiler.java" 7176]
  [clojure.lang.Compiler eval "Compiler.java" 7131]
  [clojure.core$eval invokeStatic "core.clj" 3214]
  [clojure.core$eval invoke "core.clj" 3210]
  [clojure.main$repl$read_eval_print__9068$fn__9071 invoke "main.clj" 414]
  [clojure.main$repl$read_eval_print__9068 invoke "main.clj" 414]
  [clojure.main$repl$fn__9077 invoke "main.clj" 435]
  [clojure.main$repl invokeStatic "main.clj" 435]
  [clojure.main$repl_opt invokeStatic "main.clj" 499]
  [clojure.main$main invokeStatic "main.clj" 598]
  [clojure.main$main doInvoke "main.clj" 561]
  [clojure.lang.RestFn invoke "RestFn.java" 397]
  [clojure.lang.AFn applyToHelper "AFn.java" 152]
  [clojure.lang.RestFn applyTo "RestFn.java" 132]
  [clojure.lang.Var applyTo "Var.java" 705]
  [clojure.main main "main.java" 37]]}

Those are java stacktraces, even for code running inside Clojure. I believe many developers would benefit from stacktraces elements originated from Clojure being rendered in Clojure-aware way. For example:

[clojure.core/eval "core.clj" 3214]
  [clojure.main/repl/read-eval-print "main.clj" 414]
  [clojure.main/repl "main.clj" 435]
  [clojure.main/repl-opt "main.clj" 499]
  [clojure.main/main "main.clj" 598]
  [clojure.main/main "main.clj" 561]

Key differences:

  • Names demunged (user sees names as they appear in Clojure file, not how they got compiled to Java code. Not every Clojure user need to be aware of that part, I believe)
  • Duplicate invoke/invokeStatic/doInvoke collapsed (again, this is an implementation detail, from Clojure user calling a method is a single operation. As a result, many stacktraces might become quite shorter)

I believe there are many more implementation details like protocol calls, multimethod calls, anonymous functions, where compilation details spill into stacktraces, blow their size and make them only readable by compiler experts. Although not specified in this ticket, those should be collapsed to clojure-friendly representation as well.

There are also many places where such representation should be applied:

  • Default exception printer
  • clojure.main
  • clojure.repl
  • clojure.stacktrace (used by clojure.test)

Also, maybe all those places need some sort of unification? E.g. why are there three (root-cause) implementations in clojure.core?

Prior work: I believe (pst) does some basic demunging, but not collapsing. It also doesn't handle complex cases of protocols etc. And it has to be explicitly called, which is not very convenient.






[CLJ-2348] [spec] 'check' has inconsistent behavior for required and optional map keys Created: 16/Apr/18  Updated: 15/Dec/18

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

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

org.clojure/spec.alpha "0.1.143"


Approval: Triaged

 Description   

Repro:

(s/def :ex/f fn?)
(s/def :ex/m (s/keys :opt [:ex/f]))
(s/fdef my-fn
        :args (s/cat :m :ex/m))
(defn my-fn [f])
(clojure.spec.test.alpha/check `my-fn)

Actual: Exception is thrown - "Unable to construct gen at: [:m :ex/f] for: :ex/f"

Expected: A value should be returned containing the failure. This is the behavior that will occur if you replace the ":opt" with a ":req" in the keys spec.

I would expect this value to contain a failure such that:

(ex-data (:result (:clojure.spec.test.check/ret (first (clojure.spec.test.alpha/check `my-fn))))) ;; => #:clojure.spec.alpha{:path [:m :ex/f], :form :ex/f, :failure :no-gen}





[CLJ-2455] Doc that rseq works on colls satisfying reversible? Created: 12/Dec/18  Updated: 12/Dec/18

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Approval: Triaged

 Description   

The current docstring of rseq indicates that its parameter can be a vector or sorted map:

user=> (doc rseq)
-------------------------
clojure.core/rseq
([rev])
  Returns, in constant time, a seq of the items in rev (which
  can be a vector or sorted-map), in reverse order. If rev is empty returns nil

But, rseq also works on, say, sorted sets.

Request that the docstring be updated to indicate that rseq works on any rev satisfying reversible?.






[CLJ-1811] test line reporting doesn't always report test's file & line number Created: 02/Sep/15  Updated: 12/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: Ryan Fowler Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: clojure.test

Attachments: Text File clj-1811.patch     File error_reporting.clj     Text File LINE_REPORING_1.patch     Text File LINE_REPORING_2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

If an Exception is thrown during test execution, the filename and line number are frequently not helpful for finding the problem. For instance, this code:

error_reporting.clj
(require '[clojure.test :refer [deftest test-var]])

(deftest foo
  (meta))

(test-var #'foo)

Will output an error at AFn.java:429.

ERROR in (foo) (AFn.java:429)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ArityException: Wrong number of args (0) passed to: core/meta--4144
 at clojure.lang.AFn.throwArity (AFn.java:429)
    clojure.lang.AFn.invoke (AFn.java:28)
    user/fn (error_reporting.clj:4)
    clojure.test$test_var$fn__7670.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)
    user$eval6.invoke (error_reporting.clj:6)
    clojure.lang.Compiler.eval (Compiler.java:6782)
    ...etc

Rich's Comment 24016 on CLJ-377 says that he thinks the message should report the test file line rather than where the exception was thrown.

Approach: Filter the stacktrace class prefix clojure.lang.AFn from the top of error stacktraces.

After applying the patch, the above example outputs error_reporting.clj:4:

ERROR in (foo) (error_reporting.clj:4)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ArityException: Wrong number of args (0) passed to: core/meta--4141
 at clojure.lang.AFn.throwArity (AFn.java:429)
    clojure.lang.AFn.invoke (AFn.java:28)
    user$fn__3.invokeStatic (error_reporting.clj:4)
    user/fn (error_reporting.clj:3)
    clojure.test$test_var$fn__114.invoke (test.clj:705)
    clojure.test$test_var.invokeStatic (test.clj:705)
    clojure.test$test_var.invoke (test.clj:-1)
    user$eval6.invokeStatic (error_reporting.clj:6)
    user$eval6.invoke (error_reporting.clj:-1)
    clojure.lang.Compiler.eval (Compiler.java:6939)
    ...etc

Patch: clj-1811.patch



 Comments   
Comment by Ryan Fowler [ 02/Sep/15 5:36 PM ]

example file from description

Comment by Alex Miller [ 04/Sep/15 10:04 AM ]

A quick search on Github shows many cases where people call into the (admittedly private) file-and-line function. These users would be broken by the patch. Perhaps it would be better to create a new function or a new arity rather than removing the existing arity.

Just eyeballing it, but I suspect you've introduced reflection in a couple places in the new code, particularly these might need another type hint:

1. (.getName (.getClass (:test (meta test-var))))
2. #(= (.getClassName %) test-var-class-name)

I need to look at more of the code to make a judgement on everything else. Seeing testing-vars in there means that this function is now dependent on external state, so need to think carefully to be sure that every calling context has that global state (or won't fail in bad ways if it doesn't). It would be helpful to see a discussion of your thinking about that in the "approach" section of the ticket description.

Comment by Ryan Fowler [ 30/Sep/15 3:41 PM ]

Second attempt at a patch to resolve this issue. Corrects issues Alex pointed out

Comment by Ryan Fowler [ 30/Sep/15 3:53 PM ]

I've filled in some detail in the approach section.

I also added a new patch LINE_REPORTING_2.patch that addresses reflection warnings, restores the old arity of file-and-line and adds protection from people calling file-and-line from outside a testing context.

Comment by Ryan Fowler [ 30/Sep/15 6:20 PM ]

While discussing an issue with my coworker James, I realized that this fix helps with shared functions calling (is). Notice how the run of this sample code reports line 7 with LINE_REPORTING_2.patch applied. This test line is generally much more useful than the shared function line.

example_2
ryans-mbp:~/oss/clojure% cat -n error_reporting.clj
     1  (require '[clojure.test :refer [deftest test-var is]])
     2
     3  (defn shared-code [arg]
     4    (is arg))
     5
     6  (deftest test-shared-code
     7    (shared-code false))
     8
     9  (test-var #'test-shared-code)
ryans-mbp:~/oss/clojure% java -jar ~/.m2/repository/org/clojure/clojure/1.7.0/clojure-1.7.0.jar ./error_reporting.clj

FAIL in (test-shared-code) (error_reporting.clj:4)
expected: arg
  actual: false
ryans-mbp:~/oss/clojure% java -jar target/clojure-1.8.0-master-SNAPSHOT.jar ./error_reporting.clj

FAIL in (test-shared-code) (error_reporting.clj:7)
expected: arg
  actual: false
Comment by Michael Blume [ 02/Dec/15 5:55 PM ]

Patch doesn't apply anymore, but maybe this has been sorted by CLJ-1856?

Comment by Alex Miller [ 03/Dec/15 9:57 AM ]

This is not fixed by CLJ-1856, but could be if clojure.lang.AFn was filtered out of error stacktraces when finding the location. This is pretty specific - it might make sense to broaden to other calling paths too, not sure.

Attached a new patch that applies this filtering.





[CLJ-1908] Add clojure.test api to run single test with fixtures and report Created: 01/Apr/16  Updated: 12/Dec/18

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

Type: Feature Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: clojure.test

Attachments: Text File CLJ-1908-3.patch    
Approval: Screened

 Description   

When developing code, it is sometimes effective to focus on a single failing test, rather than running all tests in a namespace. This can be the case when running the tests takes some amount of time, or when running the tests produces a large volume of failures. The best option for running a single test with fixtures currently is `test-vars` ala:

(use 'clojure.test) 
(def counter (atom 0)) 
(defn setup [f] (swap! counter inc) (f)) ;; a :once fixture with state 
(use-fixtures :once setup) 
(deftest ex (println "counter =" @counter))

(test-vars [#'ex])  ;=> counter = 1 
(test-vars [#'ex])  ;=> counter = 2

However, this has the following issues:

  • No test reporting feedback such as you get with run-tests (on success, there is no output)
  • Need to specify var (not symbols) wrapped in a vector

Proposed: A new macro `run-test` that specifies a single symbol and does the same test reporting you get with `run-tests`. Usage:

(use 'clojure.test) 
(def counter (atom 0)) 
(defn setup [f] (swap! counter inc) (f)) ;; a :once fixture with state 
(use-fixtures :once setup) 
(deftest ex (println "counter =" @counter)) 

(run-test ex)

;=> Testing user
;=> counter = 1

;=> Ran 1 tests containing 0 assertions.
;=> 0 failures, 0 errors.
;=> {:test 1, :pass 0, :fail 0, :error 0, :type :summary}

(run-test ex)

;=> Testing user
;=> counter = 2

;=> Ran 1 tests containing 0 assertions.
;=> 0 failures, 0 errors.
;=> {:test 1, :pass 0, :fail 0, :error 0, :type :summary}

Patch: CLJ-1908-3.patch

Screened: Alex Miller



 Comments   
Comment by Howard Lewis Ship [ 01/Apr/16 4:12 PM ]

Having trouble with the patch, in that, things that work at the REPL fail when executed via `mvn test`. Tracking down why is taking some time.

Comment by Howard Lewis Ship [ 01/Apr/16 4:40 PM ]

Initial patch; code works but mvn test fails and I haven't figured out why.

Comment by Howard Lewis Ship [ 01/Apr/16 5:44 PM ]

Thanks to Hiredman, was provided with insight that back ticks needed due to how Maven/Ant runs the tests. All tests now pass.

Comment by Alex Miller [ 01/Apr/16 6:43 PM ]

As far as I can tell, this is basically the same intent as CLJ-866 which was completed in Clojure 1.6. You can do this now with test-vars:

user=> (use 'clojure.test) 
nil 
user=> (def counter (atom 0)) 
#'user/counter 
user=> (defn setup [f] (swap! counter inc) (f)) ;; a :once fixture with state 
#'user/setup user=> (use-fixtures :once setup) {:clojure.test/once-fixtures (#object[user$setup 0x7106e68e "user$setup@7106e68e"])} user=> (deftest ex (println "counter =" @counter)) #'user/ex user=> (test-vars [#'ex]) 
counter = 1 
nil 
user=> (test-vars [#'ex]) 
counter = 2 
nil
Comment by Howard Lewis Ship [ 03/Apr/16 12:27 PM ]

I think there is some advantage to being able to run the tests using is symbol, not its var. Further, the change I've suggested also returns the same kind of data that `run-tests` does.

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

Some changes needed on this patch before I will prescreen it:

  • Patch should be squashed to a single commit
  • Commit message in patch should start with "CLJ-1908"
  • Change run-test* to run-test-var
  • The docstring for run-test-var should be: "Run the test in Var v with fixtures and report." Kill the "called from" sentence".
  • The first sentence of the docstring for run-test should be: "Runs a single test in the current namespace." Remove "This is meant to be invoked interactively, from a REPL.". Last sentence is ok.
  • In run-test, replace (ns-resolve ns test-symbol) with the simpler (resolve test-symbol).
Comment by Howard Lewis Ship [ 04/Apr/16 10:52 AM ]

Thanks for the input; I'll have an updated patch shortly.

Comment by Howard Lewis Ship [ 08/Apr/16 2:51 PM ]

Updated patch, squashed and reflecting all of Alex's comments.





[CLJ-1379] Quoting of :actual form is incorrect in clojure.test :pass type maps Created: 12/Mar/14  Updated: 12/Dec/18

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

Type: Defect Priority: Minor
Reporter: Hugo Duncan Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: clojure.test
Environment:

All clojure versions


Attachments: File fix-quoting-in-pass-case.diff    
Patch: Code
Approval: Triaged

 Description   

The function symbol is not correctly quoted in the construction of the :actual value in a :pass type map for clojure.test.

It currently produces (list = 1 1) instead of (list '= 1 1) for an (is (= 1 1)) test.

I haven't been able to come up with a workaround for this.



 Comments   
Comment by Alex Miller [ 12/Dec/18 1:57 PM ]

Repro case would be useful...





[CLJ-2291] <! and >! break clojure.test/is Created: 13/Dec/17  Updated: 12/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: jcr Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: clojure.test

Approval: Triaged

 Description   

`(is (not (<! ...)))` works, but `(is (<! ...))` doesn't.

Under the hood, `is` rewrites the code in such a way that it calls `(apply <!
...)`, because `is` assumes that <! is a plain function (see: `assert-expr
:default`[1], `assert-predicate`[2] in clojure.test). It triggers the ">! used
not in (go ...) block" assertion.

[1]https://github.com/clojure/clojure/blob/2e0c0a9a89ede8221504edeb90e8c4ee6cce7e16/src/clj/clojure/test.clj#L486
[2]https://github.com/clojure/clojure/blob/2e0c0a9a89ede8221504edeb90e8c4ee6cce7e16/src/clj/clojure/test.clj#L435

Example code: https://gist.github.com/augustl/4a679dc95847db4434d0e7348651224f#file-test-cljs-L34
Macroexpansion: https://gist.github.com/amalloy/26ec8b8910c7c00bd7feaeef2307bc92#file-gistfile1-txt-L48

Workaround: make `assert-expr` aware of special core.async functions, and use
`assert-any` for those. It's reasonable since core.async is a widely-used core
library. It'd fix the particular problem described in this issue.

The correct solution would be to make <! and >! macros instead of functions.



 Comments   
Comment by Alex Miller [ 12/Dec/18 1:56 PM ]

`is` has multimethod support for custom expressions, so I think this could be patched by adding `assert-expr` methods on <! and >! (probably in a core.async namespace that provides test support).





[CLJ-1808] map-invert should use transients and reduce-kv instead of reduce Created: 30/Aug/15  Updated: 12/Dec/18

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

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

Attachments: Text File clj-1808-map-invert-should-use-reduce-kv-and-transient.patch    
Patch: Code
Approval: Screened

 Description   

Two performance enhancements on clojure.set/map-invert:

1) Use reduce-kv to avoid realizing mapentry's from input map
2) Use transients to create the output map

Perf:

(use 'criterium.core)
(require '[clojure.set :as set])
(def m1 (zipmap (range 1) (range 1)))
(def m10 (zipmap (range 10) (range 10)))
(def m100 (zipmap (range 100) (range 100)))
(def m1000 (zipmap (range 1000) (range 1000)))
(quick-bench (set/map-invert m1000))
(quick-bench (set/map-invert m100))
(quick-bench (set/map-invert m10))
(quick-bench (set/map-invert m1))

;; means before:  138 ns  1.8 µs  20.6 µs  304 µs
;; means after:   151 ns  1.3 µs   9.0 µs  126 µs

Patch: clj-1808-map-invert-should-use-reduce-kv-and-transient.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 04/Sep/15 10:42 AM ]

Would be nice to see a quick perf test that compared before/after times.





[CLJ-1905] loop should retain primitive int or float without widening Created: 29/Mar/16  Updated: 12/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: Renzo Borgatti Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: compiler, performance, primitives
Environment:

Possibly older Clojure versions (but not verified).


Attachments: Text File 0001-CLJ-1905-remove-useless-widening-on-loop-bindings.patch     Text File clj-1905-2.patch    
Patch: Code and Test
Approval: Screened

 Description   

In the following example:

(defn incrementer [n]
  (let [n (int n)]
    (loop [i (int 0)]
      (if (< i n)
        (recur (unchecked-inc-int i))
        i))))

the loop-local starts as an int when just a local but is widened to a long in the recur. It should be possible to retain the primitive int (or alternately float) type on the recur, rather than widening to long (or double).

The compiler code that is promoting the int seems to be:
https://github.com/clojure/clojure/blob/a19c36927598677c32099dabd0fdb9d3097df259/src/jvm/clojure/lang/Compiler.java#L6377-L6380

Proposed: remove useless widening on loop bindings

Patch: clj-1905-2.patch

Screened by: Alex Miller. My main open question here is: do we want to support primitive int/float loop vars?



 Comments   
Comment by Alex Miller [ 29/Mar/16 10:54 AM ]

I don't think anything but primitive longs or doubles are intended to be supported in loop/recur. Presuming that is correct, this would be the expected behavior.

The last major round of numeric/primitive refactoring made the decision to focus only on supporting primitive longs and doubles. One consequence of this is that primitive int loops are difficult to optimize - the main time I run into this is when working with Java interop in a tight loop on (for example) String, collection, or array operations (all of which are int-indexed).

Re unchecked-inc vs unchecked-inc-int, the primary reason to have these two variants is not performance but behavior. In particular, hashing operations often expect to get 32-bit int overflow semantics, not 64-bit int overflow semantics.

In summary, I think in the example given I would not write it with either int or unchecked-inc-int but with long and unchecked-inc if you are looking for best performance.

Comment by Nicola Mometto [ 29/Mar/16 11:01 AM ]

Alex Miller I don't think that's correct, as (AFAIK) it's only in fn params/returns that primitive types are supposed to be restricted to longs and doubles.
Note that for example, char, byte, boolean, short etc work just fine in both let and loop, while int and float work fine in let but not in loop.

This is caused by the following 4 lines in Compiler.java https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L6278-L6281

As far as I can tell, there's no reason for those 4 lines to be there at this point in time, and removing them makes int and float locals to be emitted correctly inside loops

This example in clojure.org itself seems to assume that ints should work in loops http://clojure.org/reference/java_interop#primitives

Also from that same paragraph:

All Java primitive types are supported
let/loop-bound locals can be of primitive types, having the inferred, possibly primitive type of their init-form

Comment by Alex Miller [ 29/Mar/16 12:07 PM ]

I agree that it should be possible to let-bound primitives of other types - I'm really talking about what happens on recur.

What would you expect to happen for a fn recur target? I wouldn't expect primitives other than long or double to work there since they can't occur in the function signature.

Note that I haven't closed this ticket, still talking through this.

Comment by Alex Miller [ 29/Mar/16 12:10 PM ]

I've definitely run into cases where creating a primitive int loop/recur would be useful for tight interop loops given how common int-indexing is in Java (some of the alioth benchmarks in particular would benefit from this). I think the argument is far weaker for float though.

Comment by Nicola Mometto [ 29/Mar/16 12:19 PM ]

I don't think we need to worry about fn recur targets at all, given that the only possible primitive bindings there are either long or double, and int/floats would get widened anyway, but good point, the tests in a patch for this ticket need to be sure that case is indeed handled.

RE: floats – I recall people complaining about bad support for floats when using clojure for graphical processing.

Even if admittedly a weak argument, I'm always of the idea that we should strike to be as consistent as possible. I don't think anybody would expect let/loop locals to behave differently, or differences between primitive types (other than the documented limitation about long/double being the only working prim types for fn arguments/return vals)

Comment by Alex Miller [ 29/Mar/16 12:30 PM ]

I'll leave this one in here but I'm going to treat it as an enhancement to current behavior. I think there's a good chance that Rich will just say this is working as intended.

I don't think the example is a very good one though and would welcome a better example. The reservations regarding unchecked-inc-int do not seem correct or valid to me (as usage should be fine on longs and is not designed for perf reasons anyways). A good example would should usage of a Java api in a loop where int-indexing and int-math gives better performance.

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

I edited the title as the bug is in `loop`, not `recur`

Comment by Nicola Mometto [ 02/Apr/16 9:55 AM ]

Attached a patch that removes the useless widenings done by the compiler on loop bindings, here's a benchmark demonstrating the speedup gained when areducing over int-arrays:

Before patch:

Clojure 1.8.0
user=> (use 'criterium.core)
nil
user=> (let [xs (int-array (range 1e6))] (bench (areduce xs i ret 0 (+ ret (aget xs i)))))
Evaluation count : 64260 in 60 samples of 1071 calls.
             Execution time mean : 954.009929 µs
    Execution time std-deviation : 20.292809 µs
   Execution time lower quantile : 926.331747 µs ( 2.5%)
   Execution time upper quantile : 1.009189 ms (97.5%)
                   Overhead used : 1.840681 ns

Found 4 outliers in 60 samples (6.6667 %)
	low-severe	 4 (6.6667 %)
 Variance from outliers : 9.4244 % Variance is slightly inflated by outliers
nil

After patch:

Clojure 1.9.0-master-SNAPSHOT
user=> (use 'criterium.core)
nil
user=> (let [xs (int-array (range 1e6))] (bench (areduce xs i ret 0 (+ ret (aget xs i)))))
Evaluation count : 68640 in 60 samples of 1144 calls.
             Execution time mean : 870.462532 µs
    Execution time std-deviation : 13.100790 µs
   Execution time lower quantile : 852.357513 µs ( 2.5%)
   Execution time upper quantile : 896.531529 µs (97.5%)
                   Overhead used : 1.844045 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil
Comment by Alex Miller [ 12/Dec/18 12:46 PM ]

-2 patch rebased to apply to master, no changes, retains attribution





[CLJ-1973] generate-proxy produces unpredictable method order in generated classes Created: 01/Jul/16  Updated: 12/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: James Carnegie Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: aot, compiler
Environment:

OSX, OpenJDK 8


Attachments: Text File CLJ-1973-v1.patch     Text File CLJ-1973-v2.patch     Text File CLJ-1973-v3.patch     Text File CLJ-1973-v4.patch     Text File CLJ-1973-v5.patch    
Patch: Code
Approval: Screened

 Description   

Using core/proxy to generate proxies for Java classes produces unpredictable method ordering in the generated class files.
This is a problem for repeatable builds (when doing AOT).

Specifically, I'm running Clojure inside Docker, and I'd like my application image layer to be as small as those produced by Java developers (using Meta-inf classpaths and a lib directory). Anyway, to get this working properly so that all dependencies (including those compiled as part of AOT) are on a separate layer, I need the output of compiling my applications' dependencies' proxies to be the same each time I run the build. This reduces build time, image push time, image pull time and container scheduling time.

Example code that exhibits the problem (you'll need to run it a few times to see the issue).

https://github.com/kipz/predictable-proxies

Cause: I've tracked it down to the use of an unsorted map here:

https://github.com/clojure/clojure/blob/master/src/clj/clojure/core_proxy.clj#L186

Approach: Use a sorted map, sorted by hash of the key (which is a vector of method name, seq of param types, and return type).

Patch: CLJ-1973-v5.patch

Screened by: Alex Miller



 Comments   
Comment by James Carnegie [ 01/Jul/16 4:19 PM ]

Patch that uses a sorted-map

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

I think you can follow the advice at http://clojure.org/guides/comparators to write a simpler comparator for this patch.

Comment by James Carnegie [ 04/Jul/16 11:24 AM ]

Simpler comparator as requested.

Comment by Alex Miller [ 04/Jul/16 12:28 PM ]

I think you lost the sorted set.

Comment by James Carnegie [ 04/Jul/16 1:06 PM ]

Copy paste between branches error. Tested this time.

Comment by James Carnegie [ 04/Jul/16 1:14 PM ]

Now with more consistent formatting of 'let'

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

While this is probably fine, it might be better to use the hash (Clojure) function rather than .hashCode (Java) function. The map itself is hashed based on the hash so that seems more appropriate.

Comment by James Carnegie [ 04/Jul/16 3:15 PM ]

As requested, using Clojure 'hash' instead.

Thanks Alex - learned about boolean comparators too!

Comment by Alex Miller [ 04/Jul/16 3:52 PM ]

Note that this ordering may still change across Clojure or JVM versions as there is no guarantee of hashing across those. Pre-screening for now.





[CLJ-2286] Update `clj` REPL with hint: (use Ctrl-D to exit) Created: 11/Dec/17  Updated: 12/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: Alan Thompson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: repl
Environment:

All


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

 Description   

Update clojure.main REPL for the `clj` tool with a hint for terminating the session via Ctrl-D, like so (changes in bold):

~ > clj
Clojure 1.9.0 (use Ctrl-D to exit)
user=>
~ >

The lein repl accepts `exit`, `quit`, and Crtl-D to exit; the `clj` repl accepts only Crtl-D. A small hint upon startup on the proper way to terminate the repl session will smooth the experience for users expecting for their old habits to work.

Patch: clj-2286.patch






[CLJ-2452] filterv docstring prohibits using pred with side effects Created: 09/Dec/18  Updated: 09/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: David Bürgin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Approval: Triaged

 Description   

The docstring of filterv prohibits using a filter predicate that has side effects:

Returns a vector of the items in coll for which (pred item) returns logical true. pred must be free of side-effects.

The last statement seems unmotivated. filterv is based on reduce, and therefore eager like into or mapv. There is no obvious reason to forbid ('must'!) side effects in the predicate.

This docstring has caused confusion in the past. Examples:

The docstring was originally copied verbatim from lazy filter (CLJ-847, commit ec59eba), which could explain why the sentence is there in the first place.

Solution

Delete the sentence 'pred must be free of side-effects.' in the docstring.






[CLJ-2287] Add clojure.set/union, intersection, difference to core.spec.alpha Created: 12/Dec/17  Updated: 07/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-2287-add-clojure-set-specs-v1.patch    

 Description   

Several tickets have been created suggesting that several functions in the clojure.set namespace might throw an exception if given non-sets as arguments, because in some cases they return unexpected values. This list is a sample, not intended to be complete: CLJ-810, CLJ-1682, CLJ-1953, CLJ-1954

Now that clojure.spec exists, documenting the expected argument types of these functions can be done more precisely. The specs could be used to dynamically detect incorrect argument types passed to these functions during testing, and via any other ways built on top of clojure.spec in the future.

Alex Miller suggested in a Slack discussion that perhaps it would be helpful if contributors would help implement such specs for functions within Clojure.

The approach taken with the proposed patch is simply to spec that the :args of clojure.set/subset?, superset?, union, intersection, and difference must all be sets, and spec the :ret types of the first two as boolean, and the last three as set. This seems to match all known comments given when declining previous tickets proposing changes to the behavior of these functions.

Patch: *CLJ-2287-add-clojure-set-specs-v1.patch*

An alternate approach not taken with the patch above would be to also allow nil as arguments and return values, but it is not clear whether that is part of the intended contract of these functions, or an incidental aspect of their current implementations.

I (Andy Fingerhut) looked through all of Clojure and contrib projects for occurrences of clojure.set/union, difference, intersection, superset?, and subset?, and there is only one such call that I can quickly determine does not pass sets to them. It is the one in clojure.data/diff for which there is a patch that can correct this (assuming it is considered a bug) on this ticket: CLJ-1087

Many such calls I could quickly determine always passed sets (e.g. because the arguments were wrapped in (set ...) calls). On a small fraction of them, I gave up trying to figure it out after a minute, since determining the answer fully from code inspection would require going back up the call tree a ways.

Please suggest any other ways that these changes could be evaluated in order to increase your confidence that they are correct.



 Comments   
Comment by Alex Miller [ 12/Dec/17 1:37 PM ]

For the moment I’ll say sure. I think this particular example is a good one.

I’m not sure that all the specs here are as obvious as you expect, or at least that’s been my experience elsewhere.

I’d expect these specs to be in clojure.set.specs namespace btw.

Comment by Andy Fingerhut [ 13/Dec/17 4:33 PM ]

Attached CLJ-2287-add-clojure-set-specs-v1.patch dated 2017-Dec-13. These are my first Clojure specs ever, so feel free to go crazy on suggesting improvements.

I used the namespace clojure.set.specs.alpha rather than clojure.set.specs as you suggested, but I can easily change that if you really did want it without the .alpha

Comment by Andy Fingerhut [ 13/Dec/17 4:34 PM ]

If there is some way I can exercise the :args specs with a large suite of tests, let me know how and I can try it.

And clearly I am putting in at least for :args and :ret pretty obvious choices of specs that I believe are correct, given issues raised against those functions in the past. I don't see how they could be any more subtle than that for these functions, but let me know if I'm missing anything, e.g. the Clojure core team has decided to support non-set arguments for these functions.

I agree that some of the other functions in the clojure.set namespace might be a bit trickier to spec the args of.

Comment by Andy Fingerhut [ 14/Dec/17 8:33 PM ]

I looked through all of Clojure and contrib projects for occurrences of clojure.set/union, difference, intersection, superset?, and subset?, and there is only one such call that I can quickly determine does not pass sets to them. It is the one in clojure.data/diff for which there is a patch that can 'correct' this (assuming it is considered a bug) on this ticket: https://dev.clojure.org/jira/browse/CLJ-1087

Many such calls I could quickly determine always passed sets (e.g. because the arguments were wrapped in (set ...) calls). On a small fraction of them, I gave up trying to figure it out after a minute, since determining the answer fully from code inspection would require going back up the call tree a ways.

Comment by Andy Fingerhut [ 30/Jul/18 7:06 PM ]

If you want an off-the-shelf compatible replacement for clojure.set functions that are identical in behavior, except they perform run-time type checks of the arguments you provide to them, and throw an exception if they have the wrong types (e.g. not sets for union, intersection, difference, subset?, and superset?), consider using the fungible library: https://github.com/jafingerhut/funjible

Comment by Andy Fingerhut [ 07/Dec/18 1:08 PM ]

This Github repository implements some ideas for specs of Clojure core functions: https://github.com/slipset/speculative

There is some discussion on this issue for what kinds of violations of some clojure.set function specs have been seen in some collections of tests. I have not seen a brief summary of all of the data yet. Just wanted to have a link to it from this issue for reference: https://github.com/slipset/speculative/issues/161





[CLJ-2451] Add convenience parse-int, parse-float, parse-short, etc. functions Created: 07/Dec/18  Updated: 07/Dec/18

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

Type: Feature Priority: Major
Reporter: Orestis Markou Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: newbie


 Description   

Rationale

Based on my own experience, and also by observing the beginners channel in Slack, an extremely common pitfall for newcomers is wondering how can you parse a string to an int.

This goes wrong in number of ways:

  • People discover the int function, whose docstring is Coerce to int – that throws when passed a string.
  • People are suggested Integer/parseInt, that does the job, but then can't be used as (map Integer/parseInt coll-of-strings – this is surprising, because it's their first experience with Java interop.
  • People are suggested #(Integer/parseInt %), that does what's expected, but whose syntax can be a little bit too much for your first 10 minutes of Clojure.
  • Sometimes people are suggested #(Integer. %), which is deprecated in Java 11.
  • People go through the same procedure in ClojureScript, and have to select a different interop way to do this.

Proposal 1

Consider adding a parse-* family of functions in Clojure that are thin wrappers over the Java interop. See Appendix for a possible list.

Proposal 2

Consider exposing clojure.lang.LispReader.matchNumber as parse-number.

People can then use the various coercions functions to get back the precision that they need. This might fit better the rationale of this ticket, which is to make a very common "toy program" operation smoother for beginners, and matching the Reader's behaviour will be the least surprising thing.

People who are sensitive about performance should know more about the intricacies of boxed arithmetic on the JVM anyway. This is also pleasantly platform-agnostic, CLJS could expose match-number.

Questions/Alternatives

  • Should the functions return primitives or boxed values?
  • What should be the handling of strings like "0xff"? The parseFoo family of functions rejects those, but 0xff can be read by the Clojure reader.
  • OTOH, the decode family of functions handle some prefixes, but they return a boxed value. But they also accept numbers like #10 which is an invalid Clojure literal.

Appendix

A hopefully complete list of primitive-returning functions (as of JVM 8) is:

name args ret-value
parse-int s int
parse-int s, radix int
parse-uint s int
parse-uint s, radix int
parse-long s long
parse-long s, radix long
parse-ulong s long
parse-ulong s, radix long
parse-short s short
parse-short s, radix short
parse-byte s byte
parse-byte s, radix byte
parse-float s float
parse-double s double

The unsigned functionality was added in Java 8, so should be safe to use in newer Clojure versions. Newer JVM versions add support for parsing parts of CharSequences.






[CLJ-2169] conj has out-of-date :arglists Created: 27/May/17  Updated: 06/Dec/18

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

Type: Defect Priority: Minor
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: docstring

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

 Description   

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

Prescreened by: Alex Miller



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

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

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





[CLJ-1452] clojure.core/*rand* for seedable randomness Created: 20/Jun/14  Updated: 06/Dec/18

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

Type: Feature Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: math

Attachments: Text File CLJ-1452.patch    
Approval: Triaged

 Description   

Clojure's random functions currently use Math.random and related features, which makes them impossible to seed. This seems like an appropriate use of a dynamic var (compared to extra arguments), since library code that wants to behave randomly could transparently support seeding without any extra effort.

I propose (def ^:dynamic *rand* (java.util.Random.)) in clojure.core, and that rand, rand-int, rand-nth, and shuffle be updated to use *rand*.

I think semantically this will not be a breaking change.

Criterium Benchmarks

I did some benchmarking to try to get an idea of the performance implications of using a dynamic var, as well as to measure the changes to concurrent access.

The code used is at https://github.com/gfredericks/clj-1452-tests; the raw output is in a comment.

rand is slightly slower, while shuffle is insignificantly faster. Using shuffle from 8 threads is insignificantly slower, but switching to a ThreadLocalRandom manually in the patched version results in a 2.5x speedup.

Running on my 8 core Linode VM:

Benchmark Clojure Runtime mean Runtime std dev
rand 1.6.0 61.3ns 7.06ns
rand 1.6.0 + *rand* 63.7ns 1.80ns
shuffle 1.6.0 12.9µs 251ns
shuffle 1.6.0 + *rand* 12.8µs 241ns
threaded-shuffling 1.6.0 151ms 2.31ms
threaded-shuffling 1.6.0 + *rand* 152ms 8.77ms
threaded-local-shuffling 1.6.0 N/A N/A
threaded-local-shuffling 1.6.0 + *rand* 64.5ms 1.41ms

Approach: create a dynamic var *rand* and update rand, rand-int, rand-nth, and shuffle to use *rand*

Patch: CLJ-1452.patch

Screened by:



 Comments   
Comment by Gary Fredericks [ 21/Jun/14 7:50 PM ]

Attached CLJ-1452.patch, with the same code used in the benchmarks.

Comment by Gary Fredericks [ 23/Jun/14 8:34 AM ]

Should we be trying to make Clojure's random functions thread-local by default while we're mucking with this stuff? We could have a custom subclass of Random that has ThreadLocal logic in it (avoiding ThreadLocalRandom because Java 6), and make that the default value of *rand*.

Comment by Alex Miller [ 28/Dec/14 11:04 AM ]

I think the ThreadLocal question is interesting, not sure re answer.

It would be nice if the description summarized the results of the tests in a table and the criterium output was in the comments instead.

Comment by Gary Fredericks [ 30/Dec/14 1:26 PM ]

Full output from the test repo (which is summarized in the table in the description):

$ echo "Clojure 1.6.0"; lein with-profile +clj-1.6 run; echo "Clojure 1.6.0 with *rand*"; lein with-profile +clj-1452 run
Clojure 1.6.0

;;;;;;;;;;;;;;;;;;
;; Testing rand ;;
;;;;;;;;;;;;;;;;;;
WARNING: Final GC required 1.261632096547911 % of runtime
Evaluation count : 644646900 in 60 samples of 10744115 calls.
             Execution time mean : 61.297605 ns
    Execution time std-deviation : 7.057249 ns
   Execution time lower quantile : 56.872437 ns ( 2.5%)
   Execution time upper quantile : 84.483045 ns (97.5%)
                   Overhead used : 16.319772 ns

Found 6 outliers in 60 samples (10.0000 %)
    low-severe   1 (1.6667 %)
    low-mild     5 (8.3333 %)
 Variance from outliers : 75.5119 % Variance is severely inflated by outliers

;;;;;;;;;;;;;;;;;;;;;
;; Testing shuffle ;;
;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 4780800 in 60 samples of 79680 calls.
             Execution time mean : 12.873832 µs
    Execution time std-deviation : 251.388257 ns
   Execution time lower quantile : 12.526871 µs ( 2.5%)
   Execution time upper quantile : 13.417559 µs (97.5%)
                   Overhead used : 16.319772 ns

Found 3 outliers in 60 samples (5.0000 %)
    low-severe   3 (5.0000 %)
 Variance from outliers : 7.8591 % Variance is slightly inflated by outliers

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 420 in 60 samples of 7 calls.
             Execution time mean : 150.863290 ms
    Execution time std-deviation : 2.313755 ms
   Execution time lower quantile : 146.621548 ms ( 2.5%)
   Execution time upper quantile : 155.218897 ms (97.5%)
                   Overhead used : 16.319772 ns
Clojure 1.6.0 with *rand*

;;;;;;;;;;;;;;;;;;
;; Testing rand ;;
;;;;;;;;;;;;;;;;;;
Evaluation count : 781707720 in 60 samples of 13028462 calls.
             Execution time mean : 63.679152 ns
    Execution time std-deviation : 1.798245 ns
   Execution time lower quantile : 61.414851 ns ( 2.5%)
   Execution time upper quantile : 67.412204 ns (97.5%)
                   Overhead used : 13.008428 ns

Found 3 outliers in 60 samples (5.0000 %)
    low-severe   3 (5.0000 %)
 Variance from outliers : 15.7596 % Variance is moderately inflated by outliers

;;;;;;;;;;;;;;;;;;;;;
;; Testing shuffle ;;
;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 4757940 in 60 samples of 79299 calls.
             Execution time mean : 12.780391 µs
    Execution time std-deviation : 240.542151 ns
   Execution time lower quantile : 12.450218 µs ( 2.5%)
   Execution time upper quantile : 13.286910 µs (97.5%)
                   Overhead used : 13.008428 ns

Found 1 outliers in 60 samples (1.6667 %)
    low-severe   1 (1.6667 %)
 Variance from outliers : 7.8228 % Variance is slightly inflated by outliers

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 420 in 60 samples of 7 calls.
             Execution time mean : 152.471310 ms
    Execution time std-deviation : 8.769236 ms
   Execution time lower quantile : 147.954789 ms ( 2.5%)
   Execution time upper quantile : 161.277200 ms (97.5%)
                   Overhead used : 13.008428 ns

Found 3 outliers in 60 samples (5.0000 %)
    low-severe   3 (5.0000 %)
 Variance from outliers : 43.4058 % Variance is moderately inflated by outliers

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-local-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 960 in 60 samples of 16 calls.
             Execution time mean : 64.462853 ms
    Execution time std-deviation : 1.407808 ms
   Execution time lower quantile : 62.353265 ms ( 2.5%)
   Execution time upper quantile : 67.197368 ms (97.5%)
                   Overhead used : 13.008428 ns

Found 1 outliers in 60 samples (1.6667 %)
    low-severe   1 (1.6667 %)
 Variance from outliers : 9.4540 % Variance is slightly inflated by outliers
Comment by Gary Fredericks [ 30/Dec/14 1:28 PM ]

I think using a ThreadLocal is logically independent from adding *rand*, so it could be a separate ticket. I just suggested it here since it would for some uses mitigate any slowdown from *rand* but now that I'm looking at the benchmark results again the slowdown might be insignificant.

Comment by Gary Fredericks [ 30/Dec/14 5:44 PM ]

Also worth noting that (as I did in the benchmark code) with just the patch's changes (i.e., no ThreadLocal involved) users still gain the ability to do ThreadLocal manually, which is not currently possible.

Comment by Stuart Halloway [ 19/Jul/15 7:42 AM ]

workaround: data.generators provides seedable random

Comment by Ghadi Shayban [ 14/Jan/16 10:15 AM ]

Just noting, ThreadLocalRandom is >= JDK 7.

Comment by Gary Fredericks [ 06/Dec/18 9:20 AM ]

The >=JDK7 aspect of ThreadLocalRandom is no longer a problem, correct?

Comment by Alex Miller [ 06/Dec/18 12:27 PM ]

Right





[CLJ-2290] into docstring doesn't mention 0- and 1-arity versions Created: 13/Dec/17  Updated: 05/Dec/18

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

Type: Defect Priority: Trivial
Reporter: jcr Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: docs, docstring, documentation

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

 Description   

See: https://github.com/clojure/clojure/blob/clojure-1.9.0-alpha14/src/clj/clojure/core.clj#L6763

Docstring should mention that "(into coll) returns coll" and "(into) returns an empty vector" and the fact that "hence it can be used as a reducing function", clarifying the reason to use [] as the default (the same as with `conj`).



 Comments   
Comment by Alex Miller [ 05/Dec/18 11:08 PM ]

I would like to accept this patch, but I need a signed CA and a properly formatted patch.





[CLJ-2295] clojure.repl/doc repeats doc string in output for special forms in Clojure 1.9.0 Created: 15/Dec/17  Updated: 05/Dec/18

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, regression

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

 Description   

Here is output from Clojure 1.8.0:

user=> (doc if)
-------------------------
if
  (if test then else?)
Special Form
  Evaluates test. If not the singular values nil or false,
  evaluates and yields then, otherwise, evaluates and yields else. If
  else is not supplied it defaults to nil.

  Please see http://clojure.org/special_forms#if
nil

Here is the corresponding output from Clojure 1.9.0:

user=> (doc if)
-------------------------
if
  (if test then else?)
Special Form
  Evaluates test. If not the singular values nil or false,
  evaluates and yields then, otherwise, evaluates and yields else. If
  else is not supplied it defaults to nil.

  Please see http://clojure.org/special_forms#if
  Evaluates test. If not the singular values nil or false,
  evaluates and yields then, otherwise, evaluates and yields else. If
  else is not supplied it defaults to nil.
nil

This repetition only occurs when calling clojure.repl/doc or clojure.repl/print-doc for special form symbols, not for other symbols like macros and functions. It was introduced when modifying clojure.repl/print-doc when adding the ability to print specs in its output, and the fix is straightforward.

Prescreened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 15/Dec/17 6:20 PM ]

Patch CLJ-2295-v1.patch dated 2017-Dec-15 is one possible way to fix this issue. Verified that output for other cases, e.g. macros, is unchanged from Clojure 1.8.0 with these changes, except for the new Spec output, which should be kept.





[CLJ-2326] *compiler-options* docstring is missing :direct-linking Created: 21/Feb/18  Updated: 05/Dec/18

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

Type: Defect Priority: Minor
Reporter: Dominic Monroe Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File clj-2326.patch    
Patch: Code
Approval: Prescreened

 Description   

Docstring was never updated to add :direct-linking.

Proposed: Add line:

clojure.core/*compiler-options*
  A map of keys to options.
  Note, when binding dynamically make sure to merge with previous value.
  Supported options:
  :elide-meta - a collection of metadata keys to elide during compilation.
  :disable-locals-clearing - set to true to disable clearing, useful for using a debugger
  :direct-linking - set to true to use direct static invocation of functions, rather than vars
  Alpha, subject to change.

Patch: clj-2326.patch






[CLJ-2387] Port 65535 incorrectly excluded Created: 16/Aug/18  Updated: 05/Dec/18

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

Type: Defect Priority: Minor
Reporter: Keyhan Vakil Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: server
Environment:

n/a


Attachments: Text File port-65535-obo.patch     Text File port-65535-obo-v2.patch    
Patch: Code
Approval: Prescreened

 Description   

The following code is used in src/clj/clojure/core/server.clj to validate whether a port number is valid:

src/clj/clojure/core/server.clj
(defn- validate-opts
  "Validate server config options"
  [{:keys [name port accept] :as opts}]
  (doseq [prop [:name :port :accept]] (required opts prop))
  (when (or (not (integer? port)) (not (< -1 port 65535)))
(throw (ex-info (str "Invalid socket server port: " port) opts))))

However this is very slightly incorrect, since port 65535 is excluded but it is actually a valid port.

repl
user=> (defn is-invalid-port [port] (or (not (integer? port)) (not (< -1 port 65535))))
#'user/is-invalid-port
user=> (is-invalid-port 65534)
false
user=> (is-invalid-port 65535) ; should be false!
true
user=> (is-invalid-port 65536)
true

This is corroborated by ServerSocket's Javadoc:

IllegalArgumentException - if the port parameter is outside the specified range of valid port values, which is between 0 and 65535, inclusive.

Patch: port-65535-obo-v2.patch

Prescreened: Alex Miller



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

Hi Keyhan, have you signed the Clojure Contributor Agreement? https://clojure.org/community/contributing. If not, please do so.

Another, maybe better, solution here is (<= 0 port 65535) - that reads more closely to the check.

Comment by Keyhan Vakil [ 16/Aug/18 9:00 PM ]

Yes, just earlier today.

I agree that looks better. Current patch is port-65535-obo-v2.patch.





[CLJ-2416] [core.specs] defmulti and defmethod spec Created: 09/Oct/18  Updated: 05/Dec/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: core.specs

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

 Description   

Currently defmulti and defmethod don't have specs and don't have much macro validation.

user=> (defmulti 5 class)
ClassCastException class java.lang.Long cannot be cast to class clojure.lang.IObj

Add specs for defmulti and defmethod.

After:

user=> (defmulti 5 class)
Syntax error macroexpanding clojure.core/defmulti at (1:1).
5 - failed: simple-symbol? at: [:fn-name]

Patch: clj-2416.patch






[CLJ-899] Accept and ignore colon between key and value in map literals Created: 18/Dec/11  Updated: 05/Dec/18

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

Type: Feature Priority: Major
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: reader


 Description   

Original title was 'treat colons as whitespace' which isn't a problem description but a (flawed) implementation approach

For JSON compatibility
known problems when no spaces - x:true and y:false



 Comments   
Comment by Tassilo Horn [ 23/Dec/11 3:22 AM ]

Discussed here: https://groups.google.com/d/msg/clojure/XvJUzaY1jec/l8xEwlFl8EUJ

Comment by Kevin Downey [ 11/Jan/12 2:23 PM ]

please no

Comment by Tavis Rudd [ 16/Jan/12 12:17 PM ]

Alan Malloy raises a good point in the google group discussion (https://groups.google.com/d/msg/clojure/XvJUzaY1jec/aVpWBicwGhsJ) about accidental confusion between trailing (or floating) and leading colons:
"It isn't even as simple as "letting them
be whitespace", because presumably you want (read-string "{a: b}") to
result in (hash-map 'a 'b), but (read-string "{a :b}") to result in
(hash-map 'a :b)."

This issue could be avoided by only treating a colon as whitespace when followed by a comma. As easy cut-paste of json seems the be the key motivation here, the commas are going to be there anyway: valid {"v":, 1234} vs syntax error {a-key: should-be-a-keyword}.

Comment by Alex Baranosky [ 16/Jan/12 5:23 PM ]

This would be visually confusing imo.

Comment by Laurent Petit [ 17/Jan/12 5:01 PM ]

Please, oh please, no.

Comment by Tavis Rudd [ 18/Jan/12 2:40 PM ]

Er, brain fart. I was typing faster than I was thinking and put the comma in the wrong place. In my head I meant the form following the colon would have to have a comma after it. Thus, {"a-json-key": 1234, ...} would be valid while {"a-json-key": was-supposed-to-be-a-keyword "another-json-key" foo} would complain about the colon being an Invalid Token. I don't see the need for it, however.

Comment by Joe R. Smith [ 27/Feb/12 10:55 AM ]

Clojure already has reader syntax for a map. If we support JSON, do we also support ruby map literals? Seems like this addition would only add confusion, imo, given colons are used in keywords and keywords are frequently used in maps - e.g., when de-serializing from XML, or even JSON.

Comment by David Nolen [ 27/Feb/12 11:19 AM ]

Clojure is no longer a language hosted only on the JVM. Clojure is also hosted on the CLR, and JavaScript. In particular ClojureScript can't currently easily deal with JSON literals - an extremely common (though problematic) data format. By allowing colon whitespace in map literals - Clojure data structures can effectively become an extensible JSON superset - giving the succinctness of JSON and the expressiveness of XML.

+1 from me.

Comment by Tim McCormack [ 13/Nov/12 7:27 PM ]

Clojure is only hosted on the JVM; ClojureScript is hosted on JS VMs. If this is useful for CLJS, it should just be a CLJS feature.

Comment by Mike Anderson [ 10/Dec/12 11:51 PM ]

-1 for this whole idea: that way madness lies....

If we keep adding syntactical oddities like this then the language will become unmaintainably complex. It's the exact opposite of simple to have lots of special cases and ambiguities that you have to remember.

If people want to use JSON that is fine, but then the best approach use a specific JSON parser/writer, not just paste it into Clojure source and expect it to work.

Comment by Laszlo Török [ 11/Dec/12 4:54 AM ]

-1 for reasons mentioned by Allan Malloy and Mike Anderson

Comment by Bozhidar Batsov [ 19/Oct/14 3:06 AM ]

-1 Don't repeat the mistake made in Ruby...





[CLJ-704] range function has missing documentation Created: 04/Jan/11  Updated: 05/Dec/18

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

Type: Enhancement Priority: Trivial
Reporter: Maarten Hus Assignee: Plínio Balduino
Resolution: Unresolved Votes: 0
Labels: docstring
Environment:

All


Attachments: Text File clj-704.patch    
Patch: Code
Approval: Prescreened

 Description   

The range function's documentation does indicate the following usage:

(range 10 0 -1) -> (10, 9, 8, 7, 6, 5, 4, 3, 2, 1)

Current doc:

Returns a lazy seq of nums from start (inclusive) to end
  (exclusive), by step, where start defaults to 0, step to 1, and end to
  infinity. When step is equal to 0, returns an infinite sequence of
  start. When start is equal to end, returns empty list.

Its also possible to step down rather than up, for example counting
backwards from 10 by -1: (range 10 0 -1).

Proposed:

Add final sentence: "Step may be negative."

Patch: clj-704.patch



 Comments   
Comment by Rasmus Svensson [ 15/Jan/11 7:39 AM ]

The current doc actually mentions the 'step' parameter briefly:

"[...] to end (exclusive), by step, where start [...]"

But as this might be easy to miss, an addition to the doc is still a good idea, I think.

My suggestion:

Returns a lazy seq of nums from start (inclusive) to end
(exclusive), by step, where start defaults to 0, step to 1, and end
to infinity. Step may be negative to count backwards.

Comment by Plínio Balduino [ 13/Jun/14 10:59 PM ]

There was any news about it?

Could I assign it to me?

Comment by Andy Fingerhut [ 13/Jun/14 11:11 PM ]

No, no news about this one. It is in the 'open' state, meaning that there is currently no judgement as to whether Clojure screeners or Rich Hickey are interested in such a change. http://dev.clojure.org/display/community/JIRA+workflow

That said, it seems like it should not take a lot of time to create a patch. Detailed instructions are given here: http://dev.clojure.org/display/community/Developing+Patches

Comment by Alex Miller [ 14/Jun/14 7:34 AM ]

Go for it!

Comment by Plínio Balduino [ 14/Jun/14 10:57 AM ]

I cannot reproduce this: "When step is equal to 0, returns an infinite sequence of start."

https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L2726-L2729

Is this correct?

Comment by Andy Fingerhut [ 14/Jun/14 2:47 PM ]

The last time range was modified was due to ticket CLJ-1018. See the attached patch there, which I believe is the one that was applied after Clojure 1.5 but before Clojure 1.6.

Perhaps that change does not do what it claimed to do in the doc string. I haven't looked at it in detail yet.

Comment by Andy Fingerhut [ 14/Jun/14 2:54 PM ]

In Clojure 1.6, it appears that it may be more accurate if the last two sentences in the doc string were modified. range's doc string is currently:

Returns a lazy seq of nums from start (inclusive) to end
(exclusive), by step, where start defaults to 0, step to 1, and end to
infinity. When step is equal to 0, returns an infinite sequence of
start. When start is equal to end, returns empty list.

It might be more accurate to say:

Returns a lazy seq of nums from start (inclusive) to end
(exclusive), by step, where start defaults to 0, step to 1, and end to
infinity. When start is equal to end, returns empty list. When step
is equal to 0 and start and end differ, returns an infinite sequence of
start.





[CLJ-434] Additional copy methods for URLs in clojure.java.io Created: 10/Sep/10  Updated: 05/Dec/18

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


 Description   

The copy method in clojure.java.io doesn't handle java.net.URL as input.
The necessary methods can be found in the mailing list post:
[[url:http://groups.google.com/group/clojure/browse_thread/thread/24a105b12466a8e8]]



 Comments   
Comment by Assembla Importer [ 10/Sep/10 7:32 AM ]

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





[CLJ-213] Invariants and the STM Created: 01/Dec/09  Updated: 05/Dec/18

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

Type: Feature Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

(ticket requested here http://groups.google.com/group/clojure/browse_thread/thread/119311e89fa46806/4903ce25ff6deaa6#4903ce25ff6deaa6)

The general idea is to declare invariants inside a transaction and, when at commit time an invariant doesn't hold anymore, the transaction retries.
So it can both act as a kind of soft ensure or to specify actions that "partially commute".
Thus it would enable coarser refs.

See the attached file for quick prototype.

User code would looks like:

(invariant (@world :key))
(commute world update-in [:key] val-transform-fn)

This means the commute will occur only if (@world :key) returns the same value in-transaction and at commit point.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 7:23 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/213
Attachments:
invariants.patch - https://www.assembla.com/spaces/clojure/documents/dd4kUS3MWr3QvMeJe5aVNr/download/dd4kUS3MWr3QvMeJe5aVNr





[CLJ-115] GC Issue 111: Enable naming an array parameter for areduce Created: 17/Jun/09  Updated: 05/Dec/18

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

Type: Feature Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   
Reported by bo...@boriska.com, Apr 28, 2009

Currently there is no way to access anonymous array parameter of areduce.

Consider:

(areduce (.. System getProperties values toArray) 
     i r 0 (some_expression))
some_expression has no way to access the array.

Per Rich:
--------------------
Yes, areduce would be nicer if it looked like a binding set:

(areduce [aname anarray, ret init] expr)
(areduce [aname anarray, ret init, start-idx  start-n] expr)
(areduce [aname anarray, ret init, start-idx  start-n, end-idx end-n]
expr) 
--------------------

This was discussed here:
http://groups.google.com/group/clojure/tree/browse_frm/thread/40597a8ac322bc37/8cf6b17328ea7e8b?rnum=1&_done=%2Fgroup%2Fclojure%2Fbrowse_frm%2Fthread%2F40597a8ac322bc37%2F8cf6b17328ea7e8b%3Ftvc%3D1%26pli%3D1%26#doc_9ea7e3c5d500ed3c


 Comments   
Comment by Assembla Importer [ 24/Aug/10 5:45 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 5:45 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)





[CLJ-112] GC Issue 108: All Clojure interfaces should specify CharSequence instead of String when possible Created: 17/Jun/09  Updated: 05/Dec/18

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

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   
Reported by redchin, Apr 20, 2009

rhickey: unlink: then just use a map {:escaped true :val "foo"}

unlink: What I meant is, everything in between would want to see something 
String-y, not caring whether it's a String or MyString.

hiredman: unlink: if you use something that implements CharSequence and 
IMeta (I think it's IMeta) you get something that is basically a String, 
but with metadata

rhickey: what hiredman said

hiredman: ideally most things would not specify String but CharSequence in 
their interface

hiredman: but somehow I doubt that is case

unlink: ok.

unlink: Good to know.

rhickey: hiredman: unfortunately that's not true of some of Clojure - could 
you enter an issue for it please - use CharSequence when possible?


 Comments   
Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)





[CLJ-15] Incremental hashcode calculation for collections Created: 17/Jun/09  Updated: 05/Dec/18

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

Type: Feature Priority: Minor
Reporter: Rich Hickey Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: performance

Attachments: File lazy-incremental-hashes.diff    

 Description   
Reported by richhickey, Dec 17, 2008
So hachCode can be final, more efficient to calc as you go.
Formerly Google Code Issue 11


 Comments   
Comment by Assembla Importer [ 24/Aug/10 3:44 AM ]

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

Comment by Christophe Grand [ 08/Mar/13 6:20 AM ]

Wouldn't the naive approach incur realizing lazy sequences when adding them to a list or a vector or as values in a map?

Comment by Christophe Grand [ 26/Aug/13 3:40 AM ]

The lazy-incremental-hashes.diff introduces lazy incremental hashes based on structural sharing.

Comment by Christophe Grand [ 26/Aug/13 3:42 AM ]

Why is this identified as Blocker?

Comment by Christophe Grand [ 26/Aug/13 3:43 AM ]

setting priority to minor

Comment by Andy Fingerhut [ 26/Aug/13 7:46 AM ]

I've seen this "edit a ticket, it changes to Priority=Blocker" behavior before. I believe some of the older tickets have no Priority field at all, and when you edit any of their properties, it creates the priority field with a default value of Blocker.

Comment by Alex Miller [ 26/Aug/13 8:06 AM ]

Yes, concur with Andy's explanation on priority change. I just bulk-edited all open CLJ tickets with null priority and set their priority.

Comment by Andy Fingerhut [ 30/Jan/14 8:01 PM ]

Patch lazy-incremental-hashes.diff still applies cleanly as of Jan 30 2014 latest Clojure master, but now fails tests due to recent commits involving hash changes. I have not checked how difficult or easy it might be to update the patch to pass tests again.

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

Patch lazy-incremental-hashes.diff dated Aug 26 2013 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch.

Comment by Andy Fingerhut [ 05/Sep/14 11:41 AM ]

Patch lazy-incremental-hashes.diff dated Aug 26 2013 applies cleanly again to latest Clojure master as of Sep 5 2014, even though the patch has not been updated. I haven't checked, but I would guess this is because one of the changes made to Clojure master recently was reverted with this commit: https://github.com/clojure/clojure/commit/46be47c9f51ef10d0082f1bd39ffff1008682861





[CLJ-2] Scopes Created: 15/Jun/09  Updated: 05/Dec/18

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

Type: Feature Priority: Major
Reporter: Assembla Importer Assignee: Rich Hickey
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File scopes-spike.patch    

 Description   

Add the scope system for dealing with resource lifetime management



 Comments   
Comment by Assembla Importer [ 24/Aug/10 11:43 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 11:43 AM ]

richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)

Comment by Stuart Halloway [ 12/Jul/11 8:26 AM ]

Patch demonstrates idea, not ready for prime time.

Comment by Tassilo Horn [ 23/Dec/11 7:37 AM ]

I think the decision of having to specify either a Closeable resource or a close function for an existing non-Closeable resource in with-open is quite awkward, because they have completely different meaning.

  (let [foo (open-my-custom-resource "foo.bar")]
    (with-open [r (reader "foo.txt")
                foo #(.terminate foo)]
      (do-stuff r foo)))

I think a CloseableResource protocol that can be extended to custom types as implemented in the patch to CLJ-308 is somewhat easier to use. Extend it once, and then you can use open-my-custom-resource in with-open just like reader/writer and friends...

That said, Scopes can still be useful, but I'd vote for handling the "how should that resource be closed" question by a protocol. Then the with-open helper can simply add

(swap! *scope* conj (fn [] (clojure.core.protocols/close ~(bindings 0))))

and cleanup-scope only needs to apply each fn without having to distinguish Closeables from fns.





[CLJ-1323] AsmReflector throws exceptions on JDK8 Created: 13/Jan/14  Updated: 04/Dec/18

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File clj-1323-disable.diff     Text File clj-1323-experiment-v1.patch    

 Description   

After the commit of the updated ASM library for CLJ-713, Clojure builds and passes all tests except for one, compare-reflect-and-asm in reflect.clj.

This can be narrowed down somewhat to a difference in behavior of the following 2 forms evaluated with the latest Clojure and JDK8:

;; The following two lines work with the latest (Jan 11 2014) Clojure 1.6.0-master-SNAPSHOT
;; if run on JDK 6 or JDK 7, but throw an exception with JDK 8.

(import '[clojure.asm ClassReader ClassVisitor Type Opcodes])
(def r (ClassReader. "java.lang.Object"))

I am not certain, but from a bit of Google searching it appears that this may be a limitation of the ASM library version 4 – it throws exceptions when attempting to read class files produced by JDK 8, because of a newer classfile version number. Links that seem to support this conclusion:

http://mail-archive.ow2.org/asm/2013-02/msg00000.html

http://forge.ow2.org/tracker/index.php?func=detail&aid=316375&group_id=23&atid=350023

A couple of alternatives are:

(1) update ASM again to one that supports JDK 8 class files

(2) disable the compare-reflect-and-asm test. Clojure itself does not use the AsmReflector for anything except this unit test. The Java reflector is the default one.



 Comments   
Comment by Alex Miller [ 13/Jan/14 8:16 AM ]

1) There is no released ASM that supports JDK 8 yet. ASM 5 will but it will not be final till JDK 8 is in the final stages of release.

2) Probably more likely.

Comment by Nicola Mometto [ 19/Mar/14 9:01 AM ]

As of now, both JDK8 and ASM5 are out.
I just tried compiling clojure on JDK8 with ASM5 and all compiles fine

Comment by Alex Miller [ 19/Mar/14 9:07 AM ]

How are you running this test?

Comment by Nicola Mometto [ 19/Mar/14 9:11 AM ]

I downloades ASM5, replaced the bundled ASM that comes with clojure with that one after changing che package name to "clojure.asm" and run `mvn install`, all the tests pass.

Comment by Alex Miller [ 19/Mar/14 9:24 AM ]

I was actually talking about the JDK 8 change - was curious about exactly what was being changed?

Comment by Alex Miller [ 19/Mar/14 10:04 AM ]

In particular, I'm assuming that you're not altering the build.xml to change the compilation -source or -target and running with JAVA_HOME / path set to JDK 8.

We don't have any plans to actually build Clojure with JDK 8 any time soon, so I'm not overly concerned about that. But it does appear that the embedded ASM 4 cannot read newer class files from JDK 8. Afaik, the only place that happens is in clojure.reflect.java in the AsmReflector, which is not the default reflector. The JavaReflector will properly reflect the Java 8 classes.

ASM 5 has only been out a couple days and already has at least one serious bug reported - I'd like that to see more use before we switch to it, so maybe this is a good target for the release after Clojure 1.6.

Comment by Alex Miller [ 23/Mar/14 12:17 PM ]

Patch to temporarily disable the failing test until we have an ASM that supports JDK 8.

Comment by Andy Fingerhut [ 04/Dec/18 12:21 AM ]

I have tested the latest Clojure 1.10.0-RC3 code, with the test that was disabled re-enabled again, using the patch in attachment clj-1323-experiment-v1.patch.

Without the change to file src/clj/clojure/reflect/java.clj, it fails when running with OpenJDK 11, because the uncommented test throws an exception when it attempts to call (type-reflect classname :reflector asm-reflector) for class java.io.FileInputStream (and perhaps others after that in the vector).

With the patch as written, all tests pass using these version combinations:
+ Mac OS X 10.13.6 plus Oracle Java 1.8.0_192
+ Ubuntu 16.04.5 plus OpenJDK 11 - the uncommented test throws

I am not saying that I know this patch is good, or the full ramifications of the change I made in file src/clj/clojure/reflect/java.clj. I was poking at the code to run an experiment to see if I could make the test that was commented out pass again. Someone like Ghadi Shayban would be better than I at assessing the ramifications of such a change.

Comment by Alex Miller [ 04/Dec/18 7:50 AM ]

Not planning to do this for Clojure 1.10 but can assess for 1.11.

Comment by Andy Fingerhut [ 04/Dec/18 11:52 AM ]

Definitely understood – no rush for 1.10 release here. Just scanning through the list of open defect tickets for the first time in a while, and seeing which ones I might have something to say a little bit about.





[CLJ-1876] calling require from java is not thread safe Created: 07/Jan/16  Updated: 04/Dec/18

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

Type: Defect Priority: Major
Reporter: Robert (Bobby) Evans Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Crappy Linux VM running RHEL6

java version "1.8.0_60"
Java(TM) SE Runtime Environment (build 1.8.0_60-b27)
Java HotSpot(TM) 64-Bit Server VM (build 25.60-b23, mixed mode)



 Description   

As a part of Apache Storm we have some code that can load a clojure function from java using the following code.

public static IFn loadClojureFn(String namespace, String name) {
        try {
            clojure.lang.Compiler.eval(RT.readString("(require '" + namespace + ")"));
        } catch (Exception e) {
            //if playing from the repl and defining functions, file won't exist
        }
        return (IFn) RT.var(namespace, name).deref();
    }

If this function is called from multiple different threads at the same time, trying to import the same namespace, I will occasionally get some very odd errors. NOTE: I had to modify the catch to actually print out the error message it was getting (We should not be eating exceptions either way).

{verbatim}
2016-01-07 16:26:09.305 b.s.u.Utils [WARN] Loading namespace failed
clojure.lang.Compiler$CompilerException: java.lang.RuntimeException: Unable to resolve symbol: sentence-spout in this context, compiling:(storm/starter/clj/word_count.clj:21:1)
at clojure.lang.Compiler.analyze(Compiler.java:6543) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6485) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3791) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6725) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6524) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6485) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.eval(Compiler.java:6786) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.load(Compiler.java:7227) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.loadResourceScript(RT.java:371) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.loadResourceScript(RT.java:362) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.load(RT.java:446) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.load(RT.java:412) ~[clojure-1.7.0.jar:?]
at clojure.core$load$fn__5448.invoke(core.clj:5866) ~[clojure-1.7.0.jar:?]
at clojure.core$load.doInvoke(core.clj:5865) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.invoke(RestFn.java:408) ~[clojure-1.7.0.jar:?]
at clojure.core$load_one.invoke(core.clj:5671) ~[clojure-1.7.0.jar:?]
at clojure.core$load_lib$fn__5397.invoke(core.clj:5711) ~[clojure-1.7.0.jar:?]
at clojure.core$load_lib.doInvoke(core.clj:5710) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.applyTo(RestFn.java:142) ~[clojure-1.7.0.jar:?]
at clojure.core$apply.invoke(core.clj:632) ~[clojure-1.7.0.jar:?]
at clojure.core$load_libs.doInvoke(core.clj:5749) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.applyTo(RestFn.java:137) ~[clojure-1.7.0.jar:?]
at clojure.core$apply.invoke(core.clj:632) ~[clojure-1.7.0.jar:?]
at clojure.core$require.doInvoke(core.clj:5832) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.invoke(RestFn.java:408) ~[clojure-1.7.0.jar:?]
at clojure.core$eval114.invoke(NO_SOURCE_FILE:0) ~[?:?]
at clojure.lang.Compiler.eval(Compiler.java:6782) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.eval(Compiler.java:6745) ~[clojure-1.7.0.jar:?]
at backtype.storm.utils.Utils.loadClojureFn(Utils.java:602) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.clojure.ClojureBolt.prepare(ClojureBolt.java:57) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.daemon.executor$fn_8297$fn_8310.invoke(executor.clj:785) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.util$async_loop$fn__556.invoke(util.clj:482) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at clojure.lang.AFn.run(AFn.java:22) [clojure-1.7.0.jar:?]
at java.lang.Thread.run(Thread.java:745) [?:1.8.0_60]
Caused by: java.lang.RuntimeException: Unable to resolve symbol: sentence-spout in this context
at clojure.lang.Util.runtimeException(Util.java:221) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.resolveIn(Compiler.java:7019) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.resolve(Compiler.java:6963) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyzeSymbol(Compiler.java:6924) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6506) ~[clojure-1.7.0.jar:?]
... 33 more{verbatim}

If I make the static java function synchronized the issue goes away. It always seems to blow up when parsing a few specific macros getting confused that a specific symbol cannot be resolved.

The namespace trying to be loaded.
https://github.com/apache/storm/blob/a99d9c11be005ade7c308bebdda71c7fb0111acc/examples/storm-starter/src/clj/storm/starter/clj/word_count.clj

The macros that we seem to get exceptions on.
https://github.com/apache/storm/blob/a99d9c11be005ade7c308bebdda71c7fb0111acc/storm-core/src/clj/backtype/storm/clojure.clj#L77-L138

And like I said it look like it is a threading issue of some sort. When I added the synchronized keyword everything works.



 Comments   
Comment by Kevin Downey [ 17/Feb/16 10:19 AM ]

calling require from clojure isn't thread safe either, no different from this

Comment by Andy Fingerhut [ 03/Dec/18 9:33 PM ]

Is this issue perhaps solved by using the new `serialized-require` that has been added in Clojure 1.10?

Comment by Alex Miller [ 04/Dec/18 7:47 AM ]

The plan is to rework require in future release so that serialized-require is no longer needed. Maybe like that, maybe some other way. This issue is basically the placeholder for the real work. Adding the lock in serialized-require is a big hammer and we would like to make that a bit more surgical.





[CLJ-1921] Wrong numeric result from Math/abs on Java 8 Created: 09/May/16  Updated: 03/Dec/18

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: math, reflection
Environment:

does not seem specific to Clojure version
occurs only in Java 1.8



 Description   

This is with Java 1.8 (Oracle or Open JDK):

weird-abs.core=> (Math/abs -10000000000000)
10000000000000
weird-abs.core=> (def a    -10000000000000)
#'weird-abs.core/a
weird-abs.core=> (Math/abs a)
1316134912

In Java 1.7, the expected results are returned instead (10000000000000).

Cause: It appears that Math.abs(int) is being invoked. Both the int and long versions are considered by the reflector but Java 1.7 and 1.8 return these signatures in different orders and the first one found is picked.

Workaround: Use hint or cast to inform the reflector which one to pick.



 Comments   
Comment by Alex Miller [ 09/May/16 9:03 AM ]

In the first case, -10000000000000 is a long and the compiler unambiguously finds Math.abs(long).

In the second case, a is an Object and all abs signatures are considered (this is in Reflector.invokeMatchingMethod). In both Java 1.7 and 1.8, the long and int signatures are found "congruent".

In Java 1.7, the long version is found first and treated as a match, then int is checked and Compiler.subsumes([int], [long]) returns false, leading to the long method being kept as the match.

In Java 1.8, the int version is found first and treated as a match, then long is checked and Compiler.subsumes([long], [int]) returns false, leading to the int method being kept as the match.

Both of these return false on both JDKs:

(Compiler/subsumes (into-array [Long/TYPE]) (into-array [Integer/TYPE]))
(Compiler/subsumes (into-array [Integer/TYPE]) (into-array [Long/TYPE]))

so the real difference is just the ordering that is considered, which is JDK-specific.

The considered signatures could be sorted in some canonical way making this behavior consistent, or could maybe express a preference between the two signatures somehow.

In any case, getting rid of the reflection here by hinting or casting a resolves the problem - it should be considered only luck not intention that the correct results comes out with Java 7 in this case, I think.

Comment by Nicola Mometto [ 10/May/16 7:58 AM ]

It seems to me that the non-deterministic behaviour of clojure's reflector of randomly picking one overload when more than one is available is both highly counterintuitive and undesirable.

IMHO the only sane approach would be to:

  • pick the most specific type if possible (e.g. if what's available is [Object, CharSequence, String] and the reflected type is a StringBuffer, we use CharSequence rather than Object.
  • pick the widest primitive type if possible (e.g. in this case we'd use long rather than int)
  • Fail with a `More than one matching method found` exception if conflicts can't be resolved (this already happens in some cases)

(I'm still scarred from previous experiences of reading/patching the complex beast that is Reflector.java and the reflective bits of Compiler.java, so I propose this with no intention of ever working on this myself )

Comment by Alex Miller [ 10/May/16 8:03 AM ]

I think the subsumes check is effectively trying to do your option #1 already - this is a case where the types of the arguments in the two cases have no hierarchical relationship. Probably #2 would make more sense - expressing a preference, although there are certainly cases where "widest" has no meaning, so not sure what the general form of this is.

Comment by Nicola Mometto [ 10/May/16 8:05 AM ]

To clarify, that wasn't a list of different options, it was a list of steps to take.
i.e. if it's possible to pick the most specific type from a hierarchy, do that, ELSE if the types are primitive, pick the widest ELSE fail

Comment by Andy Fingerhut [ 03/Dec/18 9:32 PM ]

Possibly the same as, or at least some commonality with, CLJ-1212.





[CLJ-1212] Silent truncation/downcasting of primitive type on reflection call to overloaded method (Math/abs) Created: 28/May/13  Updated: 03/Dec/18

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

Type: Defect Priority: Major
Reporter: Matthew Willson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: primitives, typehints
Environment:

Clojure 1.5.1
OpenJDK Runtime Environment (IcedTea6 1.12.5) (6b27-1.12.5-0ubuntu0.12.04.1)



 Description   

I realise relying on reflection when calling these kinds of methods isn't a great idea performance-wise, but it shouldn't lead to incorrect or dangerous behaviour.

Here it seems to trigger a silent downcast of the input longs, giving a truncated integer output:

user> (defn f [a b] (Math/abs (- a b)))
Reflection warning, NO_SOURCE_PATH:1:15 - call to abs can't be resolved.
#'user/f
user> (f 1000000000000 2000000000000)
727379968
user> (class (f 1000000000000 2000000000000))
java.lang.Integer
user> (defn f [^long a ^long b] (Math/abs (- a b)))
#'user/f
user> (f 1000000000000 2000000000000)
1000000000000
user> (class (f 1000000000000 2000000000000))
java.lang.Long



 Comments   
Comment by Matthew Willson [ 28/May/13 12:50 PM ]

For an even simpler way to replicate the issue:

user> (#(Math/abs %) 1000000000000)
Reflection warning, NO_SOURCE_PATH:1:3 - call to abs can't be resolved.
727379968

Comment by Andy Fingerhut [ 28/May/13 1:36 PM ]

I was able to reproduce the behavior you see with these Java 6 JVMs on Ubuntu 12.04.2:

java version "1.6.0_27"
OpenJDK Runtime Environment (IcedTea6 1.12.5) (6b27-1.12.5-0ubuntu0.12.04.1)
OpenJDK 64-Bit Server VM (build 20.0-b12, mixed mode)

java version "1.6.0_45"
Java(TM) SE Runtime Environment (build 1.6.0_45-b06)
Java HotSpot(TM) 64-Bit Server VM (build 20.45-b01, mixed mode)

However, I tried two Java 7 JVMs, and it gave the following behavior which looks closer to what you would hope for. I do not know what is the precise difference between Java 6 and Java 7 that leads to this behavior difference, but this is some evidence that this has something to do with Java 6 vs. Java 7.

user=> (set! warn-on-reflection true)
true
user=> (defn f [a b] (Math/abs (- a b)))
Reflection warning, NO_SOURCE_PATH:1:15 - call to abs can't be resolved.
#'user/f
user=> (f 1000000000000 2000000000000)
1000000000000
user=> (class (f 1000000000000 2000000000000))
java.lang.Long

Above behavior observed with Clojure 1.5.1 on these JVMs:

Ubuntu 12.04.2 plus this JVM:
java version "1.7.0_21"
Java(TM) SE Runtime Environment (build 1.7.0_21-b11)
Java HotSpot(TM) 64-Bit Server VM (build 23.21-b01, mixed mode)

Mac OS X 10.8.3 plus this JVM:
java version "1.7.0_15"
Java(TM) SE Runtime Environment (build 1.7.0_15-b03)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)

Comment by Matthew Willson [ 29/May/13 5:17 AM ]

Ah, interesting.
Maybe it's a difference in the way the reflection API works in java 7?

Here's the bytecode generated incase anyone's curious:

public java.lang.Object invoke(java.lang.Object);
Code:
0: ldc #14; //String java.lang.Math
2: invokestatic #20; //Method java/lang/Class.forName:(Ljava/lang/String;)Ljava/lang/Class;
5: ldc #22; //String abs
7: iconst_1
8: anewarray #24; //class java/lang/Object
11: dup
12: iconst_0
13: aload_1
14: aconst_null
15: astore_1
16: aastore
17: invokestatic #30; //Method clojure/lang/Reflector.invokeStaticMethod:(Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/Object;
20: areturn

Comment by Matthew Willson [ 29/May/13 5:20 AM ]

Just an idea (and maybe this is what's happening under java 7?) but given it's a static method and all available overloaded variants are presumably known at compile time, perhaps it could generate code along the lines of:

(cond
(instance? Long x) (Math/abs (long x))
(instance? Integer x) (Math/abs (int x))
;; ...
)

Comment by Andy Fingerhut [ 29/May/13 3:19 PM ]

In Reflector.java method invokeStaticMethod(Class c, String methodName, Object[] args) there is a call to getMethods() followed by a call to invokeMatchingMethod(). getMethods() returns the 4 java.lang.Math/abs methods in different orders on Java 6 and 7, causing invokeMatchingMethod() to pick a different one on the two JVMs:

java version "1.6.0_39"
Java(TM) SE Runtime Environment (build 1.6.0_39-b04)
Java HotSpot(TM) 64-Bit Server VM (build 20.14-b01, mixed mode)

user=> (pprint (seq (clojure.lang.Reflector/getMethods java.lang.Math 1 "abs" true)))
(#<Method public static int java.lang.Math.abs(int)>
#<Method public static long java.lang.Math.abs(long)>
#<Method public static float java.lang.Math.abs(float)>
#<Method public static double java.lang.Math.abs(double)>)
nil

java version "1.7.0_21"
Java(TM) SE Runtime Environment (build 1.7.0_21-b11)
Java HotSpot(TM) 64-Bit Server VM (build 23.21-b01, mixed mode)

user=> (pprint (seq (clojure.lang.Reflector/getMethods java.lang.Math 1 "abs" true)))
(#<Method public static double java.lang.Math.abs(double)>
#<Method public static float java.lang.Math.abs(float)>
#<Method public static long java.lang.Math.abs(long)>
#<Method public static int java.lang.Math.abs(int)>)
nil

That might be a sign of undesirable behavior in invokeMatchingMethod() that is too dependent upon the order of methods given to it.

As you mention, type hinting is good for avoiding the significant performance hit of reflection.

Comment by Alex Miller [ 02/Aug/15 8:24 PM ]

Not reproducible on 1.8.0-alpha3.

Comment by Nicola Mometto [ 03/Aug/15 9:52 AM ]

Alex, I could reproduce using 1.8.0-master-SNAPSHOT and jdk 1.8:

[~]> java -version
java version "1.8.0_45"
Java(TM) SE Runtime Environment (build 1.8.0_45-b14)
Java HotSpot(TM) 64-Bit Server VM (build 25.45-b02, mixed mode)
[~]> clj
Clojure 1.8.0-master-SNAPSHOT
user=> (set! *warn-on-reflection* true)
true
user=> (#(Math/abs %) 1000000000000)
Reflection warning, NO_SOURCE_PATH:2:3 - call to static method abs on java.lang.Math can't be resolved (argument
727379968
user=> (class *1)
java.lang.Integer

Andy's last comment mentions that clojure.lang.Reflector.invokeStaticMethod is dependant on the order of methods passed to it and that that order can change between jdk versions so maybe that's why you couldn't reproduce it

Comment by Andy Fingerhut [ 03/Dec/18 9:31 PM ]

Possibly the same as, or at least some commonality with, CLJ-1921.





[CLJ-2445] Use "constructor" instead of "ctor" in Compiler error messages Created: 26/Nov/18  Updated: 03/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: Jakub Holý Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

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

 Description   

In this example:

user=> (java.net.URL. #_"missing arg")
Syntax error (IllegalArgumentException) compiling new at (REPL:1:1).
No matching ctor found for class java.net.URL

the use of "ctor" is confusing. It matches the code variable name, but the user isn't looking at that.

Patch: CLJ-2445.patch - replace "ctor" with "constructor" in error messages.

Prescreened by: Alex Miller



 Comments   
Comment by Jakub Holý [ 26/Nov/18 8:00 AM ]

patch CLJ-2445.patch added 11/26

Comment by Alex Miller [ 26/Nov/18 9:59 AM ]

Can you add a repro example to the description?

Comment by Jakub Holý [ 03/Dec/18 11:23 AM ]

add repro example to descr.

Comment by Alex Miller [ 03/Dec/18 12:09 PM ]

Removed the second example as it's the same as the first (which was from your original, but with 1.10 result printing, which is where we're starting from now).





[CLJ-2426] satisfies? doesn't work with the new instance-based protocol polymorphism Created: 06/Nov/18  Updated: 29/Nov/18

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

Type: Defect Priority: Major
Reporter: Juraj Martinka Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: protocols

Attachments: Text File 0001-CLJ-2426-fix-satisfies-for-new-instance-based-protoc.patch    
Patch: Code
Approval: Triaged

 Description   

I'd expect that `satisfies?` works even when providing protocol implementation via metadata, however that doesn't seem to be the case:

(defprotocol Foo :extend-via-metadata true (foo [x]))
(foo (with-meta [42] {`foo (fn [x] :boo)}))
;; => :boo

;; but `satisfies?` doesn't work
(satisfies? Foo (with-meta [42] {`foo (fn [x] :boo)}))
;; => false

Patch: 0001-CLJ-2426-fix-satisfies-for-new-instance-based-protoc.patch



 Comments   
Comment by Alex Miller [ 29/Nov/18 8:08 PM ]

Would it be better to put the metadata check second instead of first for perf? Or is it actually faster?





[CLJ-1095] Allow map-indexed to accept multiple collections (a la map) Created: 25/Oct/12  Updated: 29/Nov/18

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

Type: Feature Priority: Trivial
Reporter: Bo Jeanes Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: None

Attachments: Text File 0001-map-indexed-accepts-multiple-collections.patch     Text File 0002-Add-test-for-multi-collection-map-indexed-fn.patch     Text File CLJ-1601 map-indexed and tests 8-5-2017.patch    
Approval: Vetted

 Description   

Bring external interface of map-indexed in line with map. Existing usages of map-indexed unchanged both in implementation and interface.

examples
(map vector (range 10 20) (range 30 35)) ;=> ([10 30] [11 31] [12 32] [13 33] [14 34])
(map-indexed vector (range 10 20) (range 30 35)) ;=> ([0 10 30] [1 11 31] [2 12 32] [3 13 33] [4 14 34])

Patch: CLJ-1601 map-indexed and tests 8-5-2017.patch - also adds tests for map-indexed

Prescreened by: Alex Miller



 Comments   
Comment by Aaron Bedra [ 25/Oct/12 5:11 PM ]

Can you add a test for the improved functionality?

Comment by Bo Jeanes [ 25/Oct/12 5:20 PM ]

You bet. I tried to before submitting this but found no existing tests for map-indexed to expand upon. Given that, I decided to just start the conversation first. If you think this is a good addition, I'll find a place to stick the tests and add a new patch file.

Comment by Bo Jeanes [ 25/Oct/12 8:05 PM ]

Add two unit tests for map-indexed. One tests old behavior (single collection) and the second tests mapping across 3 collections.

There were no existing tests for map-indexed that I could see to expand upon (using git grep map-indexed src/clojure)

Comment by Justin Spedding [ 31/Jul/17 9:38 PM ]

This feature would be very useful. Is there a reason that no one continued to work on this in almost 5 years?

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

Predates my involvement with Clojure so don't know but don't see any reason why not.

Comment by Andy Fingerhut [ 31/Jul/17 10:36 PM ]

If a ticket has not been moved to the Vetted state by Rich Hickey, it is unknown whether it is of interest to be added to Clojure at all. The default answer to any change to Clojure, until there is a compelling argument to the core Clojure developers otherwise, is "no". That said, JIRA tickets tend to be left open rather than declined if there is no compelling argument for declining it.

For some more background, see here, in particular the "Clojure Governance and How It Got That Way" article linked from there: https://dev.clojure.org/display/community/Contributing+FAQ

For details on Vetted, and other JIRA ticket statuses used by the Clojure core team, see here: https://dev.clojure.org/display/community/JIRA+workflow

Voting on tickets (there is a "Vote" link near the top right of the page if you log in to your JIRA account first) can in some cases help get attention to them sooner than tickets without votes, but there are no guarantees on time lines.

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

I didn't try but I'm sure this patch no longer applies. Having a single combined patch that can be applied would be a useful first step.

Comment by Justin Spedding [ 05/Aug/17 7:40 PM ]

I just uploaded a new patch for map-indexed that allows it to accept multiple collections. I took the exact same approach that map does, but with the addition of the index. It has better performance than the old patch from 2012. I also included more tests. Both the function update and the new tests are in "CLJ-1601 map-indexed and tests 8-5-2017.patch"

Also, map-indexed had 0 tests prior to me writing the patch. That seems a bit dangerous for a function in core to be untested.

Comment by Justin Spedding [ 04/Oct/17 11:37 AM ]

It's been a little while since I uploaded the patch here and it was prescreened. Both this ticket and another that I created are awaiting vetting, and the FAQ says that I should tend to my tickets. Is there something else that I should be doing to tend to it and move it along?

Comment by Alex Miller [ 04/Oct/17 2:37 PM ]

I don't think we'll consider any more enhancements in 1.9 timeframe.

Comment by Justin Spedding [ 29/Nov/18 12:16 PM ]

It occurs to me that `keep` and `keep-indexed` also do not accept multiple collections. Perhaps I should make a similar ticket for updating those.

Comment by Alex Miller [ 29/Nov/18 12:19 PM ]

Yes, separate ticket for those would be good. I don’t know of one in existence yet.





[CLJ-1005] Use transient map in zipmap Created: 30/May/12  Updated: 29/Nov/18

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

Type: Enhancement Priority: Major
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 17
Labels: ft, performance

Attachments: Text File 0001-Use-transient-map-in-zipmap.2.patch     Text File 0001-Use-transient-map-in-zipmap.patch     Text File 0002-CLJ-1005-use-transient-map-in-zipmap.patch     Text File CLJ-1005-zipmap-iterators.patch    
Patch: Code
Approval: Vetted

 Description   

#'zipmap constructs a map without transients, where transients could improve performance.

Approach: Use a transient map internally, along with iterators for the keys and values. A persistent map is returned as before. The definition is also moved so that it resides below that of #'transient.

Performance:

(def xs (range 16384))
(def ys (range 16))

expression 1.7.0-beta3 +patch  
(zipmap xs xs) 4.50 ms 2.12 ms large map
(zipmap ys ys) 2.75 us 2.07 us small map

Patch: CLJ-1005-zipmap-iterators.patch

Screened by: Alex Miller



 Comments   
Comment by Aaron Bedra [ 14/Aug/12 9:24 PM ]

Why is the old implementation left and commented out? If we are going to move to a new implementation, the old one should be removed.

Comment by Michał Marczyk [ 15/Aug/12 4:17 AM ]

As mentioned in the ticket description, the previously attached patch follows the pattern of into whose non-transient-enabled definition is left in core.clj with a #_ in front – I wasn't sure if that's something desirable in all cases.

Here's a new patch with the old impl removed.

Comment by Andy Fingerhut [ 15/Aug/12 10:37 AM ]

Thanks for the updated patch, Michal. Sorry to raise such a minor issue, but would you mind using a different name for the updated patch? I know JIRA can handle multiple attached files with the same name, but my prescreening code isn't quite that talented yet, and it can lead to confusion when discussing patches.

Comment by Michał Marczyk [ 15/Aug/12 10:42 AM ]

Thanks for the heads-up, Andy! I've reattached the new patch under a new name.

Comment by Andy Fingerhut [ 16/Aug/12 8:24 PM ]

Presumptuously changing Approval from Incomplete back to None after the Michal's updated patch was added, addressing the reason the ticket was marked incomplete.

Comment by Aaron Bedra [ 11/Apr/13 5:32 PM ]

The patch looks good and applies cleanly. Are there additional tests that we should run to verify that this is providing the improvement we think it is. Also, is there a discussion somewhere that started this ticket? There isn't a lot of context here.

Comment by Michał Marczyk [ 11/Apr/13 6:19 PM ]

Hi Aaron,

Thanks for looking into this!

From what I've been able to observe, this change hugely improves zipmap times for large maps. For small maps, there is a small improvement. Here are two basic Criterium benchmarks (transient-zipmap defined at the REPL as in the patch):

;;; large map
user=> (def xs (range 16384))
#'user/xs
user=> (last xs)
16383
user=> (c/bench (zipmap xs xs))
Evaluation count : 13920 in 60 samples of 232 calls.
             Execution time mean : 4.329635 ms
    Execution time std-deviation : 77.791989 us
   Execution time lower quantile : 4.215050 ms ( 2.5%)
   Execution time upper quantile : 4.494120 ms (97.5%)
nil
user=> (c/bench (transient-zipmap xs xs))
Evaluation count : 21180 in 60 samples of 353 calls.
             Execution time mean : 2.818339 ms
    Execution time std-deviation : 110.751493 us
   Execution time lower quantile : 2.618971 ms ( 2.5%)
   Execution time upper quantile : 3.025812 ms (97.5%)

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 25.4675 % Variance is moderately inflated by outliers
nil

;;; small map
user=> (def ys (range 16))
#'user/ys
user=> (last ys)
15
user=> (c/bench (zipmap ys ys))
Evaluation count : 16639020 in 60 samples of 277317 calls.
             Execution time mean : 3.803683 us
    Execution time std-deviation : 88.431220 ns
   Execution time lower quantile : 3.638146 us ( 2.5%)
   Execution time upper quantile : 3.935160 us (97.5%)
nil
user=> (c/bench (transient-zipmap ys ys))
Evaluation count : 18536880 in 60 samples of 308948 calls.
             Execution time mean : 3.412992 us
    Execution time std-deviation : 81.338284 ns
   Execution time lower quantile : 3.303888 us ( 2.5%)
   Execution time upper quantile : 3.545549 us (97.5%)
nil

Clearly the semantics are preserved provided transients satisfy their contract.

I think I might not have started a ggroup thread for this, sorry.

Comment by Andy Fingerhut [ 03/Sep/14 8:10 PM ]

Patch 0001-Use-transient-map-in-zipmap.2.patch dated Aug 15 2012 does not apply cleanly to latest master after some commits were made to Clojure on Sep 3 2014.

I have not checked whether this patch is straightforward to update. See the section "Updating stale patches" at http://dev.clojure.org/display/community/Developing+Patches for suggestions on how to update patches.

Comment by Michał Marczyk [ 14/Sep/14 12:48 PM ]

Thanks, Andy. It was straightforward to update – an automatic rebase. Here's the updated patch.

Comment by Ghadi Shayban [ 22/Sep/14 9:58 AM ]

New patch using clojure.lang.RT/iter, criterium shows >30% more perf in the best case. Less alloc probably but I didn't measure. CLJ-1499 (better iterators) is related

Comment by Justin Spedding [ 29/Nov/18 12:08 PM ]

4 years later, this zipmap implementation in the core namespace is still not using a transient map internally. Is there a reason why this was never applied?

Comment by Alex Miller [ 29/Nov/18 12:18 PM ]

Multiple approaches proposed here, no consensus approach determined yet. Needs some focus time to narrow that down, and hasn’t been a priority yet.





[CLJ-2251] [spec] Generic spec walking for clojure.spec Created: 11/Oct/17  Updated: 27/Nov/18

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

Type: Feature Priority: Major
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Unresolved Votes: 17
Labels: None
Environment:

[org.clojure/spec.alpha "0.1.134"]



 Description   

Problem

To do runtime coercion, specs need to be walked twice to strip away the branching information: s/conform + s/unform. This introduced extra latency (see the sample below).

Proposal

New versatile s/walk* to support generic spec walking.

Current status

Still, when running s/conform + s/unform, we walk the specs twice - which is performance-wise suboptimal. Below is a sample, with Late 2013 MacBook Pro with 2,5 GHz i7, with JVM running as -server.

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

(s/def ::id int?)
(s/def ::name string?)
(s/def ::languages (s/coll-of #{:clj :cljs} :into #{}))
(s/def ::street string?)
(s/def ::zip string?)
(s/def ::number int?)

(s/def ::address (s/keys
                   :req-un [::street ::zip ::number]))

(s/def ::user (s/keys
                :req [::id]
                :req-un [::name ::address]
                :opt-un [::languages]))

(def value {::id 1
            :name "Liisa"
            :languages #{:clj :cljs}
            :address {:street "Hämeenkatu"
                      :number 24
                      :zip "33200"}})

; 2.0 µs
(cc/quick-bench
  (s/conform ::user value))

; 6.2 µs
(cc/quick-bench
  (s/unform ::user (s/conform ::user value)))

Despite s/conform is relatively fast, we triple the latency in the sample when running also s/unform. As we know already that we are not interested in the branching info, we could just not emit those.

Suggestion

s/walk* to replace both s/confrom* and s/unform*, maybe even s/explain*. It would take extra mode argument, which would be a Keyword of one of the following:

  • :validate - return false on first failing spec
  • :conform - like the current s/conform*, maybe also return s/explain results?
  • :unform - like the current s/unform*
  • :coerce - s/conform* + s/unform*, could be optimized (e.g. if no branching info, just return the value)

The public apis could be remain the same (+ optional extra argument with CLJ-2116), and a new s/coerce to call the s/walk* with :coerce.

Results

Single sweep validation & coercion. Happy runtime.



 Comments   
Comment by Tommi Reiman [ 17/Apr/18 7:40 AM ]

Renamed the issue. Instead of Keyword argument, it should take a function to walk the spec to support arbitrary walking applications.

Comment by Marco Molteni [ 10/Jul/18 5:11 PM ]

hello, any news ?

Comment by Alex Miller [ 10/Jul/18 6:20 PM ]

No plans to look at this before the next batch of implementation changes, so it will be a while.





[CLJ-2413] Non-deterministic method selection during reflection Created: 04/Oct/18  Updated: 26/Nov/18

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

Type: Defect Priority: Critical
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reflection

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

 Description   

Reflection does not return the same order of methods across JVM invocations (via Class::getMethods). This is probably most apparent when submitting Clojure functions, which are Callable and Runnable, into an executor:

(ns repro)

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

(defn repro
  []
  (let [exec (java.util.concurrent.ForkJoinPool/commonPool)
        work #(do 1)]
    (deref (.submit ^Object exec work))))  ;; intentionally reflect

(defn -main []
   ;; dereffing a runnable returns nil, a callable can return a value, in this case an integer
  (System/exit (or (repro) 0)))
nondeterministic ➜ while true; do clojure -m repro; echo $?; done
0
0
0
1
0
0
0
1
0
0
0


 Comments   
Comment by Ghadi Shayban [ 25/Nov/18 5:36 PM ]

attached a patch that sorts seen methods

Comment by Ghadi Shayban [ 26/Nov/18 8:51 AM ]

re patch: the comparator's first pass (examining the arity) can disappear: only identical arity methods get compared. Does that sound right?

Comment by Nicola Mometto [ 26/Nov/18 11:04 AM ]

Yeah, that sounds right to me

Comment by Ghadi Shayban [ 26/Nov/18 11:42 AM ]

patch with a more minimal comparator





[CLJ-2443] [spec] Spec'ed fn doesn't throw when called lazily Created: 23/Nov/18  Updated: 26/Nov/18

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

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

Attachments: Text File CLJ-2443.patch     Text File CLJ-2443-test.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Summary:

1) Given instrumented function map-f that is called lazily, e.g. as (def ls (map map-f (range))).
2) Also given instrumented function varargs-f, a varargs function. When you pass a LazySeq to varargs-f some or all elements are realized as a consequence of conforming.
3) The problem: when calling (apply varargs-f ls) some invalid calls to map-f can go unnoticed.

Repro example:

In the following code map-f is expected to throw when called with something else than a Symbol. However the call to map-f with a String slips through.

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

(defn map-f [x]
  (println "called my-fn with type" (type x))
  (println (deref #'clojure.spec.test.alpha/*instrument-enabled*))
  {x 1})

(s/fdef map-f :args (s/cat :x symbol?))

(defn varargs-f [& maps]
  true)

(s/fdef varargs-f :args (s/cat :maps (s/* map?)))

(defn repro [& args]
  (apply varargs-f (map map-f args)))

(stest/instrument)

(repro 'foo 'bar "baz")

Output:

called my-fn with type clojure.lang.Symbol
true
called my-fn with type clojure.lang.Symbol
true
called my-fn with type java.lang.String ;; <--
nil

Cause:

When varargs-f's arguments are realized as a result of conforming, some calls to map-fn are made in the scope of with-instrument-disabled: https://github.com/clojure/spec.alpha/blob/f23ea614b3cb658cff0044a027cacdd76831edcf/src/main/clojure/clojure/spec/test/alpha.clj#L140

Background:

I ran into this issue when spec'ing merge-with. Spec'ed fns in some test namespaces didn't throw anymore, because this line in clojure.test has a similar problem with regards to spec as described above: https://github.com/clojure/clojure/blob/28efe345d5e995dc152a0286fb0be81443a0d9ac/src/clj/clojure/test.clj#L775

CLJ-2443.patch contains a fix, but I realize it may not be perfect yet. Therefore I provided CLJ-2443-test.patch that only contains the unit test which can be used to test an alternative solution.



 Comments   
Comment by Michiel Borkent [ 24/Nov/18 9:50 AM ]

CLJ-2443.patch fixes this problem by conforming a Cons type argument outside the scope of with-instrument-disabled.
Test provided. Conforming this patch works with speculative (https://github.com/slipset/speculative).

Comment by Michiel Borkent [ 25/Nov/18 7:27 AM ]

CLJ-2443-test only contains the test, not the fix itself.

Comment by Michiel Borkent [ 25/Nov/18 7:29 AM ]

Updated description with CLJ-2443-test.patch





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

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

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

org.clojure/spec.alpha "0.1.143"


Approval: Triaged

 Description   

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

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

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

Repro:

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


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

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

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

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

  (my-macro "")
  @!ed

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

  ;; ^--- No caller information


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

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

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

Comment by Ben Brinckerhoff [ 19/Mar/18 6:39 PM ]

Ah, you're correct, thanks for clarifying.

If caller information was added, it would also be very useful to add a specific `:clojure.spec.alpha/failure` value for specs that fail during macro expansion. That would allow 3rd party tools to show specific types of error messages when macro expansion failed.

Comment by Ben Brinckerhoff [ 19/Mar/18 6:41 PM ]

Additionally, having access to the symbol for the macro name would assist in error reporting.

Comment by Shogo Ohta [ 27/Mar/18 9:54 PM ]

IMHO explain-data for instrumentation failures and macro spec errors should contain the fspec of the function (or macro), as I suggested before in CLJ-2218.

An fspec has some useful info (such as the ns-qualified name symbol and the line number etc.) of the spec'ed function in its metadata, so spec error reporters can use them for richer and more precise error messages in a uniform way for both instrumentation failures and macro spec errors.

Comment by Alex Miller [ 22/Aug/18 9:44 AM ]

After CLJ-2373, the caller information for spec macro failures will be conveyed in a wrapper CompilerException (as with other compiler and macroexpansion errors). Should consider whether this issue is effectively resolved as a result.

Comment by Ben Brinckerhoff [ 22/Aug/18 10:02 AM ]

I'm glad to hear this will be included in the CompilerException, but I think it still is useful to include in the spec explain-data.

When printing a spec error, it would be clearer if macro expansion spec errors and instrumentation errors could be printed in a similar format (if the user chooses a custom spec printer). It may be disruptive to look for the caller information in one place for macro expansion errors and a different place for instrumentation errors.

Furthermore, spec printers may choose to do novel things with this caller information like adding color to output or printing the actual code instead of the line numbers. It'd be nice to do have all this information available for the spec printer to customize.

Comment by Alex Miller [ 22/Aug/18 10:34 AM ]

The issue here is that at the point where the macro spec error is created, we don't have any way to know the proper caller information. The stack at that point is the compiler macroexpansion, not the invoking code. macroexpansion does know that context and that's what it adds when wrapping with a CompilerException (that's the primary purpose of CompilerException).

I think to change that we would need to convey that info out of band into the spec check. Well, maybe that's possible. I'll look at it again.

Comment by Ben Brinckerhoff [ 25/Nov/18 5:53 PM ]

With the recent error message work (great work!) the symbol appears to be available, but it not currently passed to the "explain-out" function: https://github.com/clojure/clojure/blob/b182982007df934394f0bc68b3a238ca9f200dd1/src/clj/clojure/main.clj#L268-L279

Would it be possible to "assoc" this into the "spec" passed to "spec/explain-out"? This would allow custom printers to refer to the name of the spec, which would be very useful.

An alternative approach would be for users to set up custom error handlers in the REPL, but this is much more involved than setting up a third-party implementation of "explain-out"

Comment by Alex Miller [ 26/Nov/18 6:49 AM ]

I’d hate to make this something only useable by the clojure main repl tgough - would be better to happen always if using some other repl, so would be better to do at construction. I have a better handle on how to do so now, but don’t know if it’s too late for 1.10.





[CLJ-1879] reduce-kv on hash-maps or array-maps don't consistently execute the intended fastpath Created: 09/Jan/16  Updated: 25/Nov/18

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

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

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

 Description   

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

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

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

Another approach is to extend the protocol to PersistentHashMap and PersistentArrayMap directly (in addition to preserving the existing extensions)



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

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

Comment by Ghadi Shayban [ 25/Nov/18 12:21 PM ]

Repro code

(def C (atom 0))

;; reextend for instrumentation
(extend-protocol clojure.core.protocols/IKVReduce
 ;;slow path default
 clojure.lang.IPersistentMap
 (kv-reduce 
   [amap f init]
   (swap! C inc)
   (reduce (fn [ret [k v]] (f ret k v)) init amap)))

(reduce-kv (fn [_ _ _]) nil (array-map 1 2 3 4))
(println "slowpaths" @C)
(reduce-kv (fn [_ _ _]) nil (hash-map 1 2 3 4))
(println "slowpaths" @C)

Repro output

➜  dev /usr/lib/jvm/java-8-openjdk/bin/java -cp $(clojure -Spath) clojure.main kvfastpath.clj
slowpaths 1
slowpaths 2
➜  dev /usr/lib/jvm/java-11-openjdk/bin/java -cp $(clojure -Spath) clojure.main kvfastpath.clj
slowpaths 0
slowpaths 0




[CLJ-1326] Inconsistent reflection warnings when target is a literal Created: 19/Jan/14  Updated: 22/Nov/18

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

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


 Description   
user=> (set! *warn-on-reflection* true)
true
user=> (.contains [] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false
user=> (.contains [1] 0)
false
user=> (.contains ^:foo [1] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false
user=> (.contains [(inc 1)] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false

Even worse, type hinting doesn't get picked up :

user=>  (.contains ^java.util.Collection [] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false
user=>  (.contains ^java.util.Collection [(inc 1)] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false
user=>  (.contains ^java.util.Collection (identity [(inc 1)]) 0)
false

Similar issues apply to other literals



 Comments   
Comment by Alex Miller [ 20/Nov/18 9:13 PM ]

This is not public api, does not seem worth messing with.

Comment by Nicola Mometto [ 21/Nov/18 8:47 AM ]

the clojure collections are at least documented to implement `java.util.Collection`, so the below example showcasing the same exact issue should be valid:

user=> (set! *warn-on-reflection* true)
true
user=> (.contains [] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false
user=> (.contains [1] 0)
false
user=> (.contains ^:foo [1] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false
user=> (.contains [(inc 1)] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false
Comment by Nicola Mometto [ 22/Nov/18 5:51 AM ]

I'm reopening this since the ticket is actually valid now that I've changed example – and even worse than that I noticed that it's impossible to get rid of the reflection even with explicit type hinting





[CLJ-2442] Clojure test fails on Windows 10 Created: 21/Nov/18  Updated: 21/Nov/18

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

Type: Defect Priority: Trivial
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: test
Environment:

Windows 10 with JDK 11 at least, and likely most JDKs on most versions of Windows


Attachments: Text File clj-2442-v1.patch    
Patch: Code and Test

 Description   

A test compares a string with line separators in it against a literal string with newline characters in it, which works on systems with newlines as the line terminator, but not Windows. Using clojure.string/split-lines on the literal string and the function return value before comparing fixes it, as was done for an earlier failing Clojure test a few years ago.



 Comments   
Comment by Andy Fingerhut [ 21/Nov/18 4:08 PM ]

Patch clj-2442-v1.patch fixes the failing test when building on Windows, and also passes on Mac OS X. It should work fine on Linux, too.





[CLJ-2439] The 'do symbol is skipped in some contexts where it shouldn't be Created: 20/Nov/18  Updated: 21/Nov/18

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

Type: Defect Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Attachments: Text File 0001-Don-t-skip-over-do-in-expression-context.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Expressions like this result in nil, but should fail to compile:

(let [] do)

Expressions like this result in nil, but should produce a value:

(let [do 1] do)

This is true for all special forms with an "implicit do": try/catch/finally, letfn*, let*, and the fn*/reify/deftype family of forms.

The way that "implicit do" is implemented for these special forms is that the relevant Parser delegates to BodyExpr.Parser explicitly for its body expression. For example,

(let [x 1] (println x) x)

calls BodyExpr.Parser.parse() with the list '((println x) x). However, BodyExpr.Parser is also used to parse lists like '(do (println x) x), in other contexts. To handle both cases, its parse() method skips its first form if that form is the symbol 'do. So, if a special form with an "implicit do" has as its first expression the bare symbol 'do, that symbol is incorrectly discarded.

The attached patch fixes this by making each such special form actually insert an explicit 'do before delegating, so that BodyExpr.Parser can unconditionally assume that its first form is 'do, and skip it. BodyExpr now throws an exception if this is not the case, as this would indicate an error in the compiler - user code cannot create this situation.

The attached patch will have minimal impact on compilation time: it introduces one extra method call and one extra cons cell allocation for each analysis of an expression with an implicit do. It will have no impact on generated code (except that it will fix the errors indicated in the beginning of this ticket).

Three tests are included which fail before the patch but succeed after it.



 Comments   
Comment by Alan Malloy [ 20/Nov/18 4:47 PM ]

The previous version of the patch accidentally included only the test, and not the fix. This patch contains both.

Comment by Nicola Mometto [ 21/Nov/18 8:59 AM ]

I think this may be a dupe of CLJ-1216, not sure how the two patches compare

Comment by Alan Malloy [ 21/Nov/18 3:05 PM ]

Yes, definitely a dupe of that. I like your patch better, too.





[CLJ-1419] Report errors on missing param list or return type of methods in gen-class and gen-interface Created: 10/May/14  Updated: 20/Nov/18

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

Type: Enhancement Priority: Trivial
Reporter: Nathan Zadoks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, gen-class

Attachments: Text File 0001-CLJ-1419-default-to-void-return-type-in-gen-interfac.patch     Text File 0001-CLJ-1419-map-nil-to-void-in-prim-class.patch     File clj1419.clj     Text File fail.log    
Patch: Code

 Description   

The following are invalid and should produce errors when invoked on gen-class or gen-interface:

(gen-interface :name clj1419.IFail :methods [[myMethod java.lang.String]])  ;; no params, throws error
(gen-interface :name clj1419.IFail :methods [[myMethod []]]) ;; no return type
(gen-interface :name clj1419.IFail :methods [[myMethod]])  ;; no params or return type

The first example throws an error. The second and third do not but will generate an invalid class, verify with:

(.getMethods clj1419.IFail)
ClassNotFoundException java.lang.  java.net.URLClassLoader$1.run (URLClassLoader.java:366)

Add checks to prevent these errors.



 Comments   
Comment by Nathan Zadoks [ 10/May/14 1:34 PM ]

I've implemented both fixes, and attached them as patches.

Comment by Nathan Zadoks [ 10/May/14 1:40 PM ]

I'd argue that the behaviour of asm-type is at fault here (it can output an invalid type name when passed a nil argument), so I prefer that fix over the purely symptomatic generate-interface fix.

Comment by Andy Fingerhut [ 10/May/14 2:33 PM ]

Nathan, were you planning on submitting a signed Clojure Contributor's Agreement, or already have? Details here if you have not: http://clojure.org/contributing

Patches from non-contributors cannot be committed to Clojure.

Note: I cannot promise you that one of your patches will be accepted into Clojure if you sign a CA – only that it will not if you do not sign one.

Comment by Alex Miller [ 10/May/14 4:19 PM ]

Please add an example of how this happens and the current error.

Comment by Nathan Zadoks [ 11/May/14 3:45 AM ]

Andy — Yep, I've read up on that. My CA will be underway to Rich soon. (filled in, signed, in an envelope, just need to await the arrival of those bloody international stamps…)

Alex Miller — Tahdah!

A demonstration of the issue, both attached and as a gist: https://gist.github.com/nathan7/3a7e3a09e458f1354cbb

Comment by Nathan Zadoks [ 11/May/14 3:48 AM ]

and here's log of the compiler crash that results (also added to the gist now)

Comment by Nathan Zadoks [ 11/May/14 4:27 AM ]

Whoops, both of my patches were rather broken due to a misunderstanding on my side.
I forgot entirely that asm-type takes a symbol, not a string.
Modifying asm-type was definitely a bad idea, that check just looks whether it should defer to prim->class.
Adding nil to prim->class would work (and I've attached my patch for that too), but it's starting to look rather inelegant compared to just patching gen-interface.
(on a side note: I'm having a lot of fun exploring the Clojure codebase! thanks for that, humans!)

Comment by Alex Miller [ 11/May/14 7:26 AM ]

My reading of the docstring of gen-interface is that method declarations must specify a parameter list and a valid return type. I would expect all of these to be invalid:

(gen-interface :name clj1419.IFail :methods [[fail nil]])
(gen-interface :name clj1419.IFail :methods [[fail [] nil]])
(gen-interface :name clj1419.IFail :methods [[fail []]])

"nil" is not a valid type - you can use "void" for this purpose and this works fine:

(gen-interface :name clj1419.IFail :methods [[fail [] void]])

If this ticket is (as the title states) a request to allow omitting the return type or using "nil" as a return type, then I think the answer is no. If the ticket is a request to improve the error reporting of the failure cases above, then I think we can consider that but it will be very low priority.

Comment by Nathan Zadoks [ 12/May/14 8:19 AM ]

The code seems to suggest otherwise though, seeing the explicit extra branch for pclasses being nil.
As much as I like PL trivia, I haven't run into `void` in Clojure anywhere else yet, and I'm surprised to see it here.
Maintaining the principle of least surprise seems preferable to pedantry about whether nil is a type: (= "nil" (str (type (.methodReturningVoid obj)))

Comment by Alex Miller [ 12/May/14 8:26 AM ]

The two places to look for words to rely on are docstrings and the http://clojure.org/documentation pages. Implementation details are just that.

"nil" is not a type. "void" is a documented type identifier indicating the absence of a return value - http://clojure.org/java_interop#Java%20Interop-Aliases

Comment by Nathan Zadoks [ 12/May/14 8:27 AM ]

Okay. Better error-checking in asm-type then?

Comment by Alex Miller [ 12/May/14 8:49 AM ]

I have updated the title and description based on my understanding of what this ticket should be, which is enhanced error-checking on the method specs for gen-class and gen-interface. I'm not sure if that's in asm-type or somewhere earlier.

Comment by Alex Miller [ 20/Nov/18 9:17 PM ]

As of 1.10, these all throw errors now:

user=> (gen-interface :name clj1419.IFail :methods [[myMethod java.lang.String]])
Syntax error macroexpanding gen-interface at (REPL:1:1).
Don't know how to create ISeq from: clojure.lang.Symbol

user=> (gen-interface :name clj1419.IFail :methods [[myMethod []]])
Unexpected error (ClassFormatError) macroexpanding gen-interface at (REPL:1:1).
Class name contains illegal character in descriptor in class file clj1419/IFail

user=> (gen-interface :name clj1419.IFail :methods [[myMethod]])
Unexpected error (ClassFormatError) macroexpanding gen-interface at (REPL:1:1).
Class name contains illegal character in descriptor in class file clj1419/IFail




[CLJ-1400] Error "Can't refer to qualified var that doesn't exist" should name the bad symbol Created: 09/Apr/14  Updated: 20/Nov/18

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: Compiler, errormsgs
Environment:

OS X


Attachments: File clj-1400-2.diff     File clj-1400-3.diff     File clj-1400-4.diff    
Patch: Code and Test
Approval: Incomplete

 Description   

Def of var with a ns that doesn't exist will yield this error:

user> (def foo/bar 1)
Syntax error compiling def at (REPL:1:1).
Can't create defs outside of current ns

Cause: Compiler.lookupVar() returns null if the ns in a qualified var does not exist yet.

Proposed: The error message would be improved by naming the symbol and throwing a CompilerException with file/line/col info. It's not obvious, but this may be the only case where this error occurs. If so, the error message could be more specific that the ns is the part that doesn't exist.

Patch: clj-1400-4.diff

Screened by: Alex Miller



 Comments   
Comment by Scott Bale [ 25/Jun/14 9:58 AM ]

This looks to me like relatively low hanging fruit unless I'm missing something; assigning to myself.

Comment by Scott Bale [ 26/Jun/14 11:23 PM ]

Patch clj-1400-1.diff to Compiler.java.

With this patch the example would now look like:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

I'm not sure the if(namesStaticMember(sym)) [see below], and the 2nd branch, is even necessary. Just by inspection I suspect it is not.

[footnote]

public static boolean namesStaticMember(Symbol sym){
	return sym.ns != null && namespaceFor(sym) == null;
}
Comment by Scott Bale [ 26/Jun/14 11:24 PM ]

patch: code and test

Comment by Scott Bale [ 26/Jun/14 11:27 PM ]

I tested on an actual source file, and the exception message included the file/line/col info as desired:

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)
Comment by Andy Fingerhut [ 29/Aug/14 4:46 PM ]

Patch clj-1400-1.diff dated Jun 26 2014 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch. See section "Updating Stale Patches" on this wiki page for some tips on updating patches: http://dev.clojure.org/display/community/Developing+Patches

Comment by Scott Bale [ 31/Aug/14 3:53 PM ]

Attached is an updated patch: "clj-1400-2.diff". I removed the stale patch.

Comment by Alex Miller [ 09/Sep/14 9:29 AM ]

Few comments to address:

  • Compiler diff was using spaces, not tabs, which makes it harder to diff. I attached a -3.diff that fixes this.
  • the call to namesStaticMember seems weird. The name of that method is confusing for this use. Beyond that, I think it's doing more than you need. That method is going to attempt resolve the qualified name in terms of the current ns, but I think you don't even want to do that. Rather you just want to know if the sym has a ns (sym.ns != null) - isn't that enough?
  • In what case will the other error "Var doesn't exist" occur? In other words, in what case will lookupVar not succeed in creating a new var here? If there is no such case, then remove this case. If there is such a case, then add a test.
Comment by Scott Bale [ 11/Sep/14 11:19 PM ]

Agree with all three of your bullets. Attached is an updated patch, clj-1400-4.diff.

  • I used tabs in Compiler.java
  • After close inspection of call to lookupVar(...), I believe null is returned only in the case of exactly this ticket (the symbol having a non-null namespace which has not been loaded yet). So I've taken out the conditional and the 2nd branch.
  • (Test is unchanged)
Comment by Scott Bale [ 11/Sep/14 11:22 PM ]

(properly named patch)

Comment by Alex Miller [ 11/Sep/14 11:37 PM ]

You could throw a CompilerException with the location of the problem instead (as the ticket description suggests).

Comment by Scott Bale [ 19/Sep/14 2:37 PM ]

Sorry, I should've mentioned because this wasn't obvious to me either (and in fact I forgot until just now): the RuntimeException is already caught and wrapped in a CompilerException.

I'm not sure which try-catch block within Compiler.java this is happening in, there are multiple. But you can see in the output that the exception is a CompilerException and the file|line|col info is there:

In the Repl...

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

...or in a source file

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)

Also, at the point at which the RuntimeException of this patch is being thrown, the source line and col params to CompilerException are not available, or at least not afaict.

Comment by Alex Miller [ 07/Oct/14 12:34 PM ]

I'll follow up on this patch later - Rich thought it was making too many assumptions. I think we validated many of those but need to double-check those.

Comment by Alex Miller [ 20/Nov/18 9:16 PM ]

Updated error result as of Clojure 1.10. It is now throwing a :compile-syntax error phase exception with file/line/col too.





[CLJ-2225] clojure.core/assert docstring is incorrect Created: 22/Aug/17  Updated: 20/Nov/18

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

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

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

 Description   

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

The docstring is:

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

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

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

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



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

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

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

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

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

Comment by Marc O'Morain [ 20/Nov/18 7:23 AM ]

There is one further omission in the docstring (both current and proposed) - neither mention that the behavior of `assert` depends on the value of the `clojure.core/assert` dynamic var.





[CLJ-1814] Make `satisfies?` as fast as a protocol method call Created: 11/Sep/15  Updated: 20/Nov/18

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

Type: Enhancement Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 14
Labels: performance, protocols

Attachments: Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v2.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v3.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v4.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v5.patch     Text File CLJ-1814-v6.patch     Text File CLJ-1814-v7.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Currently `satisfies?` doesn't use the same impl cache used by protocol methods, making it too slow for real world usage.

With:

(defprotocol p (f [_]))
(deftype x [])
(deftype y [])
(extend-type x p (f [_]))

Before patch:

(let [s "abc"] (bench (instance? CharSequence s))) ;; Execution time mean : 1.358360 ns
(let [x (x.)] (bench (satisfies? p x))) ;; Execution time mean : 112.649568 ns
(let [y (y.)] (bench (satisfies? p y))) ;; Execution time mean : 2.605426 µs

Cause: `satisfies?` calls `find-protocol-impl` to see whether an object implements a protocol, which checks for whether x is an instance of the protocol interface or whether x's class is one of the protocol implementations (or if its in an inheritance chain that would make this true). This check is fairly expensive and not cached.

Proposed: Extend the protocol's method impl cache to also handle (and cache) instance checks (including negative results).

After patch:

(let [x (x.)] (bench (satisfies? p x))) ;; Execution time mean : 79.321426 ns
(let [y (y.)] (bench (satisfies? p y))) ;; Execution time mean : 77.410858 ns

Patch: CLJ-1814-v7.patch (depends on CLJ-2426)



 Comments   
Comment by Michael Blume [ 11/Sep/15 4:17 PM ]

Nice. Honeysql used to spend 80-90% of its time in satisfies? calls before we refactored them out.

Comment by Michael Blume [ 24/Sep/15 3:55 PM ]

I realize this is a deeply annoying bug to reproduce, but if I clone core.match, point its Clojure dependency to 1.8.0-master-SNAPSHOT, start a REPL, connect to the REPL from vim, and reload clojure.core.match, I get

|| java.lang.Exception: namespace 'clojure.tools.analyzer.jvm.utils' not found, compiling:(clojure/tools/analyzer/jvm.clj:9:1)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5647| clojure.core$throw_if.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5733| clojure.core$load_lib.invokeStatic
|| clojure.core$load_lib.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:142)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5765| clojure.core$load_libs.invokeStatic
|| clojure.core$load_libs.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:137)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5787| clojure.core$require.invokeStatic
|| clojure.core$require.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:703)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.analyzer.jvm/0.6.5/tools.analyzer.jvm-0.6.5.jar::clojure/tools/analyzer/jvm.clj|9| clojure.tools.analyzer.jvm$eval4968$loading__5561__auto____4969.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.analyzer.jvm/0.6.5/tools.analyzer.jvm-0.6.5.jar::clojure/tools/analyzer/jvm.clj|9| clojure.tools.analyzer.jvm$eval4968.invokeStatic
|| clojure.tools.analyzer.jvm$eval4968.invoke(jvm.clj)
|| clojure.lang.Compiler.eval(Compiler.java:6934)
|| clojure.lang.Compiler.eval(Compiler.java:6923)
|| clojure.lang.Compiler.load(Compiler.java:7381)
|| clojure.lang.RT.loadResourceScript(RT.java:372)
|| clojure.lang.RT.loadResourceScript(RT.java:363)
|| clojure.lang.RT.load(RT.java:453)
|| clojure.lang.RT.load(RT.java:419)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5883| clojure.core$load$fn__5669.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5882| clojure.core$load.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5683| clojure.core$load_one.invokeStatic
|| clojure.core$load_one.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5728| clojure.core$load_lib$fn__5618.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5727| clojure.core$load_lib.invokeStatic
|| clojure.core$load_lib.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:142)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5765| clojure.core$load_libs.invokeStatic
|| clojure.core$load_libs.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:137)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5787| clojure.core$require.invokeStatic
|| clojure.core$require.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:457)
src/main/clojure/clojure/core/match.clj|1| clojure.core.match$eval4960$loading__5561__auto____4961.invoke
src/main/clojure/clojure/core/match.clj|1| clojure.core.match$eval4960.invokeStatic
|| clojure.core.match$eval4960.invoke(match.clj)
|| clojure.lang.Compiler.eval(Compiler.java:6934)
|| clojure.lang.Compiler.eval(Compiler.java:6923)
|| clojure.lang.Compiler.load(Compiler.java:7381)
|| clojure.lang.RT.loadResourceScript(RT.java:372)
|| clojure.lang.RT.loadResourceScript(RT.java:363)
|| clojure.lang.RT.load(RT.java:453)
|| clojure.lang.RT.load(RT.java:419)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5883| clojure.core$load$fn__5669.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5882| clojure.core$load.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5683| clojure.core$load_one.invokeStatic
|| clojure.core$load_one.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5728| clojure.core$load_lib$fn__5618.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5727| clojure.core$load_lib.invokeStatic
|| clojure.core$load_lib.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:142)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5765| clojure.core$load_libs.invokeStatic
|| clojure.core$load_libs.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:137)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5787| clojure.core$require.invokeStatic
|| clojure.core$require.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:421)
|| clojure.core.match$eval4949.invokeStatic(form-init2494799382238714928.clj:1)
|| clojure.core.match$eval4949.invoke(form-init2494799382238714928.clj)
|| clojure.lang.Compiler.eval(Compiler.java:6934)
|| clojure.lang.Compiler.eval(Compiler.java:6897)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|3096| clojure.core$eval.invokeStatic
|| clojure.core$eval.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|240| clojure.main$repl$read_eval_print__7404$fn__7407.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|240| clojure.main$repl$read_eval_print__7404.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|258| clojure.main$repl$fn__7413.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|258| clojure.main$repl.invokeStatic
|| clojure.main$repl.doInvoke(main.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:1523)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|58| clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__637.invoke
|| clojure.lang.AFn.applyToHelper(AFn.java:152)
|| clojure.lang.AFn.applyTo(AFn.java:144)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|645| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|1874| clojure.core$with_bindings_STAR_.invokeStatic
|| clojure.core$with_bindings_STAR_.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:425)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|56| clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invokeStatic
|| clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invoke(interruptible_eval.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|191| clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn__679$fn__682.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|159| clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__674.invoke
|| clojure.lang.AFn.run(AFn.java:22)
|| java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
|| java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
|| java.lang.Thread.run(Thread.java:745)

Same thing with reloading a namespace in my own project which depends on clojure.core.match

Comment by Nicola Mometto [ 24/Sep/15 3:59 PM ]

is it possible that AOT is involved?

Comment by Michael Blume [ 24/Sep/15 5:31 PM ]

Narrowed it down a little, if I check out tools.analyzer.jvm, open a REPL, and do (require 'clojure.tools.analyzer.jvm.utils) I get

|| java.lang.ClassCastException: java.lang.Class cannot be cast to clojure.asm.Type, compiling:(utils.clj:260:13)
|| clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3642)
|| clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3636)
|| clojure.lang.Compiler$DefExpr.eval(Compiler.java:450)
|| clojure.lang.Compiler.eval(Compiler.java:6939)
|| clojure.lang.Compiler.load(Compiler.java:7381)
|| clojure.lang.RT.loadResourceScript(RT.java:372)
|| clojure.lang.RT.loadResourceScript(RT.java:363)
|| clojure.lang.RT.load(RT.java:453)
|| clojure.lang.RT.load(RT.java:419)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5883| clojure.core$load$fn__5669.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5882| clojure.core$load.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5683| clojure.core$load_one.invokeStatic
|| clojure.core$load_one.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5728| clojure.core$load_lib$fn__5618.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5727| clojure.core$load_lib.invokeStatic
|| clojure.core$load_lib.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:142)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5765| clojure.core$load_libs.invokeStatic
|| clojure.core$load_libs.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:137)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5787| clojure.core$require.invokeStatic
|| clojure.core$require.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:421)
|| clojure.tools.analyzer.jvm.utils$eval4392.invokeStatic(form-init8663423518975891793.clj:1)
|| clojure.tools.analyzer.jvm.utils$eval4392.invoke(form-init8663423518975891793.clj)
|| clojure.lang.Compiler.eval(Compiler.java:6934)
|| clojure.lang.Compiler.eval(Compiler.java:6897)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|3096| clojure.core$eval.invokeStatic
|| clojure.core$eval.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|240| clojure.main$repl$read_eval_print__7404$fn__7407.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|240| clojure.main$repl$read_eval_print__7404.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|258| clojure.main$repl$fn__7413.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|258| clojure.main$repl.invokeStatic
|| clojure.main$repl.doInvoke(main.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:1523)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|58| clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__637.invoke
|| clojure.lang.AFn.applyToHelper(AFn.java:152)
|| clojure.lang.AFn.applyTo(AFn.java:144)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|645| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|1874| clojure.core$with_bindings_STAR_.invokeStatic
|| clojure.core$with_bindings_STAR_.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:425)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|56| clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invokeStatic
|| clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invoke(interruptible_eval.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|191| clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn__679$fn__682.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|159| clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__674.invoke
|| clojure.lang.AFn.run(AFn.java:22)
|| java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
|| java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
|| java.lang.Thread.run(Thread.java:745)

I don't see where AOT would be involved?

Comment by Nicola Mometto [ 27/Sep/15 2:28 PM ]

Michael Blume The updated patch should fix the issue you reported

Comment by Michael Blume [ 28/Sep/15 12:39 PM ]

Cool, thanks =)

New patch no longer deletes MethodImplCache, which is not used – is that deliberate?

Comment by Alex Miller [ 02/Nov/15 3:08 PM ]

It would be cool if there was a bulleted list of the things changed in the patch in the description. For example: "Renamed MethodImplCache to ImplCache", etc. That helps makes it easier to review.

Comment by Nicola Mometto [ 02/Nov/15 3:35 PM ]

Attached is an updated patch that doesn't replace MethodImplCache with ImplCache but simply reuses MethodImplCache, reducing the impact of this patch and making it easier (and safer) to review.

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

Bumping priority as this is used in new inst? predicate - see https://github.com/clojure/clojure/commit/58227c5de080110cb2ce5bc9f987d995a911b13e

Comment by Alex Miller [ 29/Jun/17 2:31 PM ]

I ran the before and after tests with the v3 patch. Before times matched pretty closely but I could not replicate the after results. I got this which is actually much worse in the not found case:

(let [x (x.)] (bench (satisfies? p x))) ;; Execution time mean : 76.833504 ns
(let [y (y.)] (bench (satisfies? p y))) ;; Execution time mean : 20.570007 µs
Comment by Nicola Mometto [ 29/Jun/17 4:09 PM ]

v4 patch fixes the regression on the not-found case, not sure how that happened, apologies.
Here are the timings I'm getting now:

clojure master:

user=> (let [x (x.)] (bench (satisfies? p x)))
Evaluation count : 604961580 in 60 samples of 10082693 calls.
             Execution time mean : 112.649568 ns
    Execution time std-deviation : 12.216782 ns
   Execution time lower quantile : 99.299203 ns ( 2.5%)
   Execution time upper quantile : 144.265205 ns (97.5%)
                   Overhead used : 1.898271 ns

Found 3 outliers in 60 samples (5.0000 %)
	low-severe	 2 (3.3333 %)
	low-mild	 1 (1.6667 %)
 Variance from outliers : 73.7545 % Variance is severely inflated by outliers
nil
user=> (let [y (y.)] (bench (satisfies? p y)))
Evaluation count : 22676100 in 60 samples of 377935 calls.
             Execution time mean : 2.605426 µs
    Execution time std-deviation : 141.100070 ns
   Execution time lower quantile : 2.487234 µs ( 2.5%)
   Execution time upper quantile : 2.873045 µs (97.5%)
                   Overhead used : 1.898271 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 40.1251 % Variance is moderately inflated by outliers
nil

master + v4:

user=> (let [x (x.)] (bench (satisfies? p x)))
Evaluation count : 731759100 in 60 samples of 12195985 calls.
             Execution time mean : 79.321426 ns
    Execution time std-deviation : 3.959245 ns
   Execution time lower quantile : 75.365187 ns ( 2.5%)
   Execution time upper quantile : 87.986479 ns (97.5%)
                   Overhead used : 1.905711 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 35.2614 % Variance is moderately inflated by outliers
nil
user=> (let [y (y.)] (bench (satisfies? p y)))
Evaluation count : 771220140 in 60 samples of 12853669 calls.
             Execution time mean : 77.410858 ns
    Execution time std-deviation : 1.407926 ns
   Execution time lower quantile : 75.852530 ns ( 2.5%)
   Execution time upper quantile : 80.759226 ns (97.5%)
                   Overhead used : 1.897646 ns

Found 4 outliers in 60 samples (6.6667 %)
	low-severe	 3 (5.0000 %)
	low-mild	 1 (1.6667 %)
 Variance from outliers : 7.7866 % Variance is slightly inflated by outliers

So to summarize:
master found = 112ns
master not-found = 2.6us

master+v4 found = 79ns
master+v4 not-found = 77ns

Comment by Michael Blume [ 14/Jul/17 5:12 PM ]

For records that have been declared with an implementation of a particular protocol, and which therefore implement the corresponding interface, would it make sense to use an (instance?) check against that interface as a fast path?

Comment by Alex Miller [ 04/Aug/17 11:28 AM ]

Michael - that check is already in there

Nicola - I have a few comments/questions:

1. I don't get what the purpose of the NIL stuff is - could you explain that?
2. In the case where x is an instance of the interface, the old code returned x and the new code in find-protocol-impl* returns interface. Why the change?
3. This: (alter-var-root (:var protocol) assoc :impl-cache (expand-method-impl-cache cache c impl)) is not thread-safe afaict - I think simultaneous misses in two different threads for different impls would cause the cache to only have one of them. This is probably unlikely, and probably not a big deal since the cache will just be updated on the next call (not give a wrong answer), but wanted to mention it. I don't see any easy way to avoid it without a lot of changes.

Comment by Nicola Mometto [ 04/Aug/17 6:14 PM ]

Alex, thanks for looking at this,
1- The NIL object is a placeholder in the method impl cache for nil, as `find-and-cache-protocol-impl` tests for `nil?` to know if the dispatch has been cached or not

2- The change is purely a consistency one, making find-and-cache-protocol-impl always return a class/interface rather than either a class/interface or a concrete instance. Behaviourally, it doesn't change anything since the two consumers of `find-protocol-impl`, namely `find-protocol-method` and `satisfies?` don't care what that value is in that case

3- Yes you are correct that it is not thread safe, however I think it's a decent tradeoff as it doesn't cause any incorrect behaviour and at worse would cause an extra cache miss, making it thread safe would mean an extra performance penalty in every cache hit/miss

Comment by Michael Blume [ 16/Aug/17 1:44 PM ]

Nicola Mometto I'm seeing a change in behavior from this patch

(defprotocol BoolProtocol
  (proto-fn [this]))

(extend-protocol BoolProtocol
  Object
  (proto-fn [x] "Object impl")

  nil
  (proto-fn [x] "Nil impl"))

(proto-fn false)

returns "Object impl" with Clojure master and "Nil impl" with this patch

Comment by Nicola Mometto [ 17/Aug/17 3:08 AM ]

That's not good, I'll take a look later today, thanks

Comment by Nicola Mometto [ 18/Aug/17 3:29 AM ]

This was an issue with how `nil` was cached, I decided to special-case `nil` to skip the method cache, removing the need for all the `NIL` funny business and fixing this bad interaction between `false` and `nil`.

Comment by Michael Blume [ 18/Aug/17 1:17 PM ]

Not sure if it's in scope for this ticket, but given that this wasn't caught, there should probably be more protocol dispatch tests

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

yes, should definitely add

Comment by Michael Blume [ 21/Aug/17 12:52 PM ]

Patch with test

Comment by Michael Blume [ 21/Aug/17 12:56 PM ]

Verified that test fails with v4 patch:

[java] Testing clojure.test-clojure.protocols
     [java]
     [java] FAIL in (test-default-dispatch) (protocols.clj:695)
     [java] expected: (= "Object impl" (test-dispatch false))
     [java]   actual: (not (= "Object impl" "Nil impl"))
     [java]
     [java] FAIL in (test-default-dispatch) (protocols.clj:695)
     [java] expected: (= "Object impl" (test-dispatch true))
     [java]   actual: (not (= "Object impl" "Nil impl"))
Comment by Michael Blume [ 18/Sep/17 2:19 PM ]

Has this patch missed the 1.9 train? There was a fix we were hoping to make in HoneySQL that I'd hesitate to make with satisfies? as slow as it is.

Comment by Alex Miller [ 18/Sep/17 2:48 PM ]

Not necessarily. We don't add features after 1.9 but perf stuff like this is still possible. It's been vetted by Rich. It's in my list of stuff to screen.

Comment by Nicola Mometto [ 20/Nov/18 7:16 AM ]

Updated patch to work after the new instance-based protocol dispatch, this ticket should wait for CLJ-2426 first





[CLJ-2431] clojure.java.io/resource NPEs when the context ClassLoader is null Created: 15/Nov/18  Updated: 15/Nov/18

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

Type: Defect Priority: Major
Reporter: mccraigmccraig Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: java.io
Environment:

clojure 1.9


Approval: Triaged

 Description   

clojure.java.io/resource assumes the current Thread's context ClassLoader will be non-null. if the Thread's context ClassLoader is null then it NPEs

(resource n (.getContextClassLoader (Thread/currentThread)))

the javadocs indicate it may reasonably be null, and i'm seeing a case in the wild where it's non-null - https://docs.oracle.com/javase/8/docs/api/java/lang/Thread.html#getContextClassLoader--

clojure.java.io/resource should perhaps default to using (ClassLoader/getSystemClassLoader) when the context ClassLoader is null






[CLJ-2333] Support java.nio.file.Path in clojure.java.io Created: 07/Mar/18  Updated: 14/Nov/18

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

Type: Enhancement Priority: Major
Reporter: Michael Nygard Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: io

Attachments: Text File CLJ-2333-1.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

java.nio.file.Path objects are largely equivalent to java.io.File. They represent a location in a (possibly-remote) filesystem.

Coercions in clojure.java.io don't recognize java.nio.file.Path. It would be nice if they did.

Prescreened: Alex Miller

Patch: CLJ-2333-1.patch



 Comments   
Comment by Andrew Oberstar [ 27/Jun/18 9:37 PM ]

Added a patch that supports java.nio.file.Path in Coercions and IOFactory. There are more places (particularly the do-copy multimethod) that might warrant Path support, but this seemed like a good start.

Note, in case there's any confusion, I have signed the CA (June 17th), though my name is not yet on the site's list of CA signers.

Comment by Arne Brasseur [ 14/Nov/18 2:19 AM ]

java.nio.file.Path was added in Java 7, which is perhaps why this change didn't make it in before. Clojure 1.10 will require Java 8, so perhaps it's time to consider this patch.





[CLJ-2428] Separate compiler exceptions into :compilation and :compile-syntax-check Created: 13/Nov/18  Updated: 13/Nov/18

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

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


 Description   

With the new error reporting phases in Clojure 1.10, we identified two separate phases - :compile-syntax-check (for syntax errors found by compiler) and :compilation for errors that occur during compilation but that are not syntax errors.

Example of the latter:

user=> a/b
Syntax error compiling at (REPL:0:0).
No such namespace: a
user=> (ex-data *e)
#:clojure.error{:phase :compile-syntax-check, :line 0, :column 0, :source "NO_SOURCE_PATH"}

The syntax here is fine, just a is not a known namespace.

The Compiler adheres to no patterns in differentiating between these two. We should go through and make this consistent, either through exception types or explicitly specifying the phase in a CompilerException.






[CLJ-2218] [spec] Improving consistency of explain-data for instrument/macroexpand-check Created: 06/Aug/17  Updated: 11/Nov/18

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

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

Attachments: Text File CLJ-2218.patch    
Patch: Code

 Description   

Description

If you instrument a function, you may get a spec error like the following:

(defn f [x] (inc x))
(s/fdef f
  :args (s/cat :x (s/and integer? even?))
  :ret (s/and integer? odd?))

(t/instrument)

(f 3)
;; ExceptionInfo Call to #'user/f did not conform to spec:
;; In: [0] val: 3 fails at: [:args :x] predicate: even?
;; :clojure.spec.alpha/spec  #object[clojure.spec.alpha$regex_spec_impl$reify__1200
 0x19b3f9a "clojure.spec.alpha$regex_spec_impl$reify__1200@19b3f9a"]
;; :clojure.spec.alpha/value  (3)
;; :clojure.spec.alpha/args  (3)
;; :clojure.spec.alpha/failure  :instrument
;; :clojure.spec.test.alpha/caller  {:file "form-init3240393046310519022.clj", :lin
e 1, :var-scope user/eval1413}
;; clojure.core/ex-info (core.clj:4725)

(ex-data *e) 
;; {:clojure.spec.alpha/problems
;;   [{:path [:args :x],
;;     :pred clojure.core/even?,
;;     :val 3,
;;     :via [],
;;     :in [0]}],
;;  :clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x19b3f9a "clojure.spec.alpha$regex_spec_impl$reify__1200@19b3f9a"],
;;  :clojure.spec.alpha/value (3),
;;  :clojure.spec.alpha/args (3),
;;  :clojure.spec.alpha/failure :instrument,
;;  :clojure.spec.test.alpha/caller {:file "form-init3240393046310519022.clj", :line 1, :var-scope user/eval1413}}

As you can see,

  • the explain-data has a regex (ie. the spec for the args of f) in it as ::s/spec
  • each problem contains :args in their :path

These facts can cause a confusion to spec error reporters because the spec for the args of f ((s/and integer? even?)) has no subspec corresponding to the key :args (I believe :path should only contains keys that is a clue to indicate which subspec to be chosen from a spec).

Possible resolutions

To resolve this confusing situation and improve the consistency of explain-data for instrument check, I think there are two options as follows:

  • Solution 1. removing :args from :path
  • Solution 2. modifying explain-data for instrument check so that they have fspec (rather than :args of it) as ::s/spec

Personally, I prefer Solution 2. since adding fspec in explain-data makes it possible to provide richer error information to *explain-out* implementors.

The same goes for macroexpand-check.



 Comments   
Comment by Alex Miller [ 06/Aug/17 11:14 PM ]

The fspec is the spec in question in here and it does have a component :args (the fspec instance supports key lookup via ILookup for :args as well). So while I would like to improve the error message and data here, I don't agree with removing :args from path. One thing that I think would be useful is for an instrumentation failure to better state the invocation (combining the function and the args into the original call). Right now those are separated and it take some mental work to knit the arg list back together.

Comment by Shogo Ohta [ 07/Aug/17 12:20 AM ]

So while I would like to improve the error message and data here, I don't agree with removing :args from path.

Yes, so once we decide to go with Solution 2., then I think there is no need to remove :args from :path.

One thing that I think would be useful is for an instrumentation failure to better state the invocation (combining the function and the args into the original call).

I totally agree that would be useful, though it sounds like it's somewhat beyond the scope of the ticket in terms of consistency improvement.

Comment by Shogo Ohta [ 24/Aug/17 5:05 AM ]

I've made a patch just to express what I meant. Any feedback will be appreciated.

Comment by Ben Brinckerhoff [ 15/Nov/17 9:02 PM ]

One thing that I think would be useful is for an instrumentation failure to better state the invocation (combining the function and the args into the original call). Right now those are separated and it take some mental work to knit the arg list back together.

Apologies if I'm missing something, but is it even possible to get the function from the explain-data? I don't see it in explain-data, but perhaps it is recoverable somehow? In any case, I would greatly appreciate it if this type of information was available, since it would make it possible to print clearer error messages.

Comment by Shogo Ohta [ 11/Nov/18 10:05 PM ]

Hey, is this already done by CLJ-2392?





[CLJ-2365] Consider integration with java.util.function interfaces Created: 26/Jun/18  Updated: 11/Nov/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 18
Labels: interop

Attachments: Text File some-java-fns-interface.patch    
Approval: Vetted

 Description   

Moving to Java 8 as a baseline allows us to consider built-in ties to critical java.util.Function interfaces like Function, Predicate, Supplier, etc. Needs assessment about what's possible and what automatic integration it would open for users.

https://docs.oracle.com/javase/8/docs/api/java/util/function/package-summary.html



 Comments   
Comment by Jason Whitlark [ 29/Jun/18 5:40 PM ]
;; I dug this out of some scratch code experimenting with kafka streams.  All the reify's were filled with java8 lambdas in the original.

;; I'll dig up another example that shows examples using stuff from java.utils.funstion.* 

;;Some of this was lifted from a franzy example or something?

;; Note that, for example, 
;; https://kafka.apache.org/0102/javadoc/org/apache/kafka/streams/kstream/Predicate.html
;; is different from
;; https://docs.oracle.com/javase/8/docs/api/java/util/function/Predicate.html

(ns utils
  (:import (org.apache.kafka.streams.kstream Reducer KeyValueMapper ValueMapper Predicate))

(defmacro reducer [kv & body]
  `(reify Reducer
     (apply [_# ~(first kv) ~(second kv)]
       ~@body)))

;; public interface KeyValueMapper<K,V,R>
;; apply(K key, V value)
(defmacro kv-mapper [kv & body]
  `(reify KeyValueMapper
     (apply [_# ~(first kv) ~(second kv)]
       ~@body)))

;; public interface ValueMapper<V1,V2>
;; apply(V1 value)
(defmacro v-mapper [v & body]
  `(reify ValueMapper
     (apply [_# ~v]
       ~@body)))

(defmacro pred [kv & body]
  `(reify Predicate
     (test [_# ~(first kv) ~(second kv)]
       ~@body)))

;; I used it something like this:

(ns our-service.kafka-streams
  (:require
   [our-service.util :as k]
   [clojure.string :as str]
  (:import 
           (org.apache.kafka.streams StreamsConfig KafkaStreams KeyValue)
           (org.apache.kafka.streams.kstream KStreamBuilder ValueMapper)))

(defn create-word-count-topology []
  (let [builder (KStreamBuilder.)
        init-stream (.stream builder (into-array ["streams-str-input"]))
        wc (-> init-stream
            (.flatMapValues (k/v-mapper [& value]
                                        (str/split (apply str value) #"\s")))
            (.map (k/kv-mapper [k v]
                               (KeyValue/pair v v)))
            (.filter (k/pred [k v]
                             (println v)
                             (not= v "the")))
            (.groupByKey)
            (.count "CountStore")
            show-item
            ;; this needs to be mapValues
            (.mapValues (reify ValueMapper
                          (apply [_ v]
                            (println v)
                            (str v))))
            (.toStream)
            (.to "wordcount-output"))]
    [builder wc]))
Comment by Ghadi Shayban [ 09/Jul/18 10:30 PM ]

The JLS infers lambda types by hunting for matching functional interfaces AKA "Single Abstract Method" classes [1] (whether they're interfaces or abstract classes.) We could have a reify-like helper that detects these classes [2]. You would have to hint the target class. We don't really need things that are both `IFn` and `j.u.f.Predicate`, etc.

(import '[java.util.function Predicate Consumer])

(let [orig [1 2 3]
      st (atom [])]
  (.forEach orig (jfn Consumer [x] (swap! st conj x)))
  (= @st orig))

[1] https://docs.oracle.com/javase/specs/jls/se8/html/jls-9.html#jls-9.8
[2] spike https://gist.github.com/ghadishayban/0ac41e81d4df02ff176c22d16ee8b972

Comment by Jason Whitlark [ 11/Jul/18 12:26 PM ]

Well, that would be an improvement. The practical problem I run into is that I'm frequently deep in a fluent interface, and don't necessarily know the exact class. That said, it's usually only in a few places. Would it make sense to have a registry? Perhaps something like:

(auto-infer-lambda [java.util.function, org.apache.kafka.streams.kstream])

Comment by Ghadi Shayban [ 11/Jul/18 3:08 PM ]

Do you ever use SAM classes that are abstract classes and not interfaces?

Comment by Andrew Oberstar [ 11/Jul/18 6:10 PM ]

Here's an alternative approach in my ike.cljj library. This uses MethodHandles (i.e. java.lang.invoke package) instead of regular reflection. I'm not sure if I tested this on abstract classes yet.

The usage looks similar to what Ghadi posted:

(defsam my-sam
  java.util.function.Predicate
  [x]
  (= x "it matched"))

(-> (Stream/of "not a match" "it matched")
    (.filter my-sam)
    (.collect Collectors/toList)

(-> (IntStream/range 0 10)
    (.filter (sam* java.util.function.IntPredicate odd?))
    (.collect Collectors/toList)

It uses MethodHandleProxies.asInterfaceInstance to create a proxy instance of the interface that calls a method handle calling a Clojure function. It doesn't try to validate parameter counts, it just treats it as varargs delegating to IFn.applyTo(ISeq). Not sure if it's the most efficient but it was effective for my needs.

I think the LambdaMetaFactory may be the preferred way to meet this type of use case. It was harder for me to follow exactly how to use that though so I didn't end up looking to deep into it.

The main functional issue I have with my approach (and Ghadi's) is that you have to explcitly provide the interface you want to proxy. Java lambdas and Groovy Closures can be used against methods that expect a SAM and it just coerces based on what the method expects. Ideally this would be supported by Clojure too.

Instead of having to do:

(-> (IntStream/range 0 10)
    (.filter (sam* java.util.function.IntPredicate odd?))
    (.collect Collectors/toList)

I'd like to do this:

(-> (IntStream/range 0 10)
    (.filter odd?)
    (.collect Collectors/toList)
Comment by Ghadi Shayban [ 07/Aug/18 2:30 PM ]

Another possible avenue is to extend java.util.function.Supplier to Clojure functions with an explicit 0-arity. That interface is becoming increasingly common in practice; it might be valuable to special-case it. (We shouldn't & couldn't do a similar thing for defrecords, as they already have a method called get, which clashes with Supplier's only method.)

Comment by Marc-André Tremblay [ 22/Sep/18 4:44 PM ]

Moving to Java 8 as a baseline allows us to use default interface methods.

The `some-java-fns-interface.patch` patch implements Consumer, Function, Predicate and Supplier on IFn.

If you want to go this route, I would be very happy to implement all interfaces under java.util.function on IFn as well as the accompanying tests. I currently use this code to play with FoundationDB through its Java client and it works well for me.

https://github.com/marctrem/clojure/commit/97742493f674edd8f6c034ee94da84fa62a76bad

Comment by Jason Whitlark [ 11/Nov/18 4:36 PM ]

If I could just use IFn's anywhere a java.util.function.* was needed, that would be fantastic!





[CLJ-2422] Confusing function name suffixes in error messages Created: 03/Nov/18  Updated: 03/Nov/18

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

Type: Enhancement Priority: Minor
Reporter: David Bürgin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting, errormsgs
Environment:

Clojure 1.10.0-beta4



 Description   

Some core function names are reported in error messages with an odd munging suffix.

For example, when passing the wrong number of arguments to inc, assoc, and dissoc, each error message reports the function name differently:

clj -Srepro -Sdeps '{:deps {org.clojure/clojure {:mvn/version "1.10.0-beta4"}}}'
user=> (inc)
Syntax error compiling inc at (1:1).
Wrong number of args (0) passed to: clojure.core/inc--inliner--5436
user=> (assoc)
Evaluation error (ArityException) at clojure.lang.AFn.throwArity (AFn.java:429).
Wrong number of args (0) passed to: clojure.core/assoc--5316
user=> (dissoc)
Evaluation error (ArityException) at clojure.lang.AFn.throwArity (AFn.java:429).
Wrong number of args (0) passed to: clojure.core/dissoc

From the user's point of view, this behaviour is unmotivated and confusing. Perhaps such names should be further demunged and printed without the suffix.



 Comments   
Comment by Alex Miller [ 03/Nov/18 1:35 PM ]

The first one here is an inlined function and happens during compilation, so is a little different than the others. Because it's inlined, it's not spec-able. The name parts there are generated within the compiler as well. This one might be fixable based on the regularity of the name construction, although I don't see it as a particularly high priority.

The latter two actually will be a bit different once we apply CLJ-2420:

user=> (assoc)
Execution error (ArityException) at user$eval154/invokeStatic (REPL:1).
Wrong number of args (0) passed to: clojure.core/assoc--5406
user=> (dissoc)
Execution error (ArityException) at user$eval156/invokeStatic (REPL:1).
Wrong number of args (0) passed to: clojure.core/dissoc

Changes here are actually eliding all the internal frames and reporting the top user frame now (here, it happens to be in the REPL invocation, but if it was in a user namespace, you'd see that instead).

I guess in both cases, the numbers you're seeing are a standard technique used throughout Clojure so the question might be whether it's possible to address this in a systematic way. Functions don't inherently carry their unmunged name right now, so demunging from class name is never going to be a foolproof mechanism.

Worth thinking about how to do this better, but not something I expect to look at soon.





[CLJ-1820] Move rename-keys from clojure.set to clojure.core Created: 28/Sep/15  Updated: 02/Nov/18

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

Type: Enhancement Priority: Trivial
Reporter: Lars Andersen Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: None


 Description   

rename-keys is hard to find when it lives in the clojure.set namespace, because it acts on maps and not sets. To my eyes set/rename-keys also looks strange when reading code, but this is the preferred way to bring in the clojure.set namespace.

This is one of the minor warts I'd like to see fixed in clojure 2.0.



 Comments   
Comment by Gordon Syme [ 04/Jul/17 4:56 AM ]

It's not just rename-keys, there are a few fns in clojure.set that don't make sense being there, at first glance anyway.

It certainly harms discoverability of these fns.

I've come across at least one re-implementation of rename-keys and map-invert during my day job because the author didn't know these fns exist.

I'd argue for breaking the relational and map fns out of clojure.set into their own namespaces and deffing some vars in clojure.set for backwards compatibility.
Those compatibility vars could be deleted in 1.10.

I'm happy to do this (or another approach) but would like some buy in on the approach from the core team first.

Comment by Nicola Mometto [ 04/Jul/17 6:25 AM ]

I think this is very, very unlikely to ever happen

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

We will not delete/move existing vars as this would break existing programs.

Comment by Marc O'Morain [ 12/Jul/17 4:49 AM ]

> We will not delete/move existing vars as this would break existing programs.

This issue can be addressed without deletion as per Gordon's suggestion:

A new var could be added to clojure.core named clojure.core/rename-keys. Then the rename-keys var in clojure.set can be defined as:

(def rename-keys clojure.core/rename-keys)
Comment by Alex Miller [ 12/Jul/17 6:08 AM ]

I am aware of that, which is why I did not close the issue. I was just stating one possible resolution that is off the table.

Comment by Bozhidar Batsov [ 30/Oct/18 4:18 PM ]

Out of curiosity - does anyone know how/why those functions ended up in the `clojure.set` namespace?

P.S. If someone decides to alias things between namespaces I'd suggest creating a `clojure.map` ns over pouring more stuff into core.

Comment by Kevin Downey [ 30/Oct/18 6:28 PM ]

a more accurate name for clojure.set based on its contents would be something like clojure.relational-algebra, where it defines something like relational algebra over sets of maps instead of over sets of tuples. A common operation in that case is renaming keys in maps (similar to using AS in a sql query). Which explains why rename-keys is in clojure.set

Comment by Andy Fingerhut [ 02/Nov/18 12:05 AM ]

Bozhidar - only a guess, not knowledge. Relations are sets of records/tuples, so to me it makes some sense that functions on relations are in clojure.set. rename-keys is probably there because it is used by two of the functions operating on relations.





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

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

Type: Defect Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: spec
Environment:

1.9.0-alpha11


Approval: Triaged

 Description   
user> (require '[clojure.spec :as s])
nil
user> (defrecord Box [a])
user.Box
user> 
user> (s/conform
        (s/cat :boxes (s/* #(instance? Box %))
               :name (s/coll-of integer?))
        [(Box. 0) [5]])
UnsupportedOperationException Can't create empty: user.Box  user.Box (form-init8049111656025227309.clj:1)
user> (clojure.repl/pst *e)
UnsupportedOperationException Can't create empty: user.Box
       	user.Box (NO_SOURCE_FILE:2)
	clojure.core/empty (core.clj:5151)
	clojure.spec/every-impl/cfns--14008/fn--14014 (spec.clj:1215)
	clojure.spec/every-impl/reify--14027 (spec.clj:1229)
	clojure.spec/conform (spec.clj:150)
	clojure.spec/dt (spec.clj:731)
	clojure.spec/dt (spec.clj:727)
	clojure.spec/deriv (spec.clj:1456)
	clojure.spec/deriv (spec.clj:1463)
	clojure.spec/deriv (spec.clj:1467)
	clojure.spec/re-conform (spec.clj:1589)
	clojure.spec/regex-spec-impl/reify--14267 (spec.clj:1633)

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



 Comments   
Comment by Alex Miller [ 02/Nov/17 3:13 PM ]

In this case, it's considering the s/* to be a non-match and then matching the (Box. 0) against (s/coll-of integer?). This matches the initial predicate (coll?) but falls through to the :else case which presumes it can call `empty`.

You can work around it by adding a :kind predicate to the coll-of:

(s/conform
        (s/cat :boxes (s/* #(instance? Box %))
               :name (s/coll-of integer? :kind #(not (record? %))))
        [(Box. 0) [5]])
;;=> {:boxes [#user.Box{:a 0}], :name [5]}
Comment by Phil Jones [ 19/Jul/18 2:41 PM ]

But why does it need to do empty in the first place?

Does this mean that you can't check collections of records in spec?

Comment by Zach Oakes [ 01/Nov/18 9:22 AM ]

I ran into this myself. Conforming a record to a coll-of spec should show a normal spec error, not throw from deep inside spec's implementation.





[CLJ-2212] docstring for `defmethod` is imprecise Created: 28/Jul/17  Updated: 31/Oct/18

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

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

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

 Description   

arglist:

(defmethod multifn dispatch-val & fn-tail)

docstring:

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

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

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

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



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

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

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

Adding patch

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

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

Comment by Marc O'Morain [ 22/Oct/18 10:38 AM ]

Adding updated patch that changes the argument name to match the docstring, rather than the other way around.

Comment by Alex Miller [ 22/Oct/18 4:48 PM ]

I'd prefer only the docstring to change here for the dispatch-val (so docstring changing to match the code).

Comment by Marc O'Morain [ 26/Oct/18 11:38 AM ]

Patch that changes only the docstring.

I've not changed the word multimethod to multifn in the docstring. I believe that even though the parameter is not documented, the name multimethod is more clear.

Can you elaborate on why you want to change the docstring rather than the argument name please Alex?

Comment by Alex Miller [ 31/Oct/18 3:57 PM ]

I changed my mind and I think it would be better to just change the docstring to match the arg names.





[CLJ-2419] Multimethods don't have the correct method name in stacktraces. Created: 22/Oct/18  Updated: 23/Oct/18

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

Type: Enhancement Priority: Minor
Reporter: Marc O'Morain Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting, multimethods


 Description   

Describing the user-facing problem:

When an exception is thrown and there is a multimethod in the stacktrace, the name of the multimethod is not shown in the callstack.

When I execute this code:

(defmulti dispatcher (fn [item] (:type item)))

(defmethod dispatcher "cat"
  [{:keys [lives]}]
  (/ lives 0))

(dispatcher {:type "cat" :lives 9})
*e

I get the following stack trace:

[clojure.lang.Numbers divide "Numbers.java" 163]
[clojure.lang.Numbers divide "Numbers.java" 3813]
[config_compilation.core$eval20467$fn__20469 invoke "/Users/marc/dev/circleci/orb-registry/server/src/config_compilation/core.clj" 198]
[clojure.lang.MultiFn invoke "MultiFn.java" 229]
... snip ...

The function name is config_compilation.core$eval20467$fn__20469.

If I use defn to declare dispatcher directly, I get the following stacktrace:

(defn dispatcher
  [{:keys [lives]}]
  (/ lives 0))

(dispatcher {:type "cat" :lives 9})
*e
[clojure.lang.Numbers divide "Numbers.java" 163]
[clojure.lang.Numbers divide "Numbers.java" 3813]
[config_compilation.core$dispatcher invokeStatic "/Users/marc/dev/circleci/orb-registry/server/src/config_compilation/core.clj" 202]
[config_compilation.core$dispatcher invoke "/Users/marc/dev/circleci/orb-registry/server/src/config_compilation/core.clj" 200]
... snip ...

The function name is given correctly as config_compilation.core$dispatcher.

I'm guessing that the issue could be that the defmulti macro not setting the :name metadata, or perhaps not passing the :name metadata to the compiler correctly.



 Comments   
Comment by Alex Miller [ 22/Oct/18 4:43 PM ]

So, it's kind of interesting if you look into the details of this. Technically, at the point where the exception occurs, the defmulti is not on the stack (that's the dispatch function, which has already completed), but the defmethod is.

defmethods are actually defined as fn's and are typically treated as anonymous fns, although it's actually possible to name them independently:

user=> (defmulti dispatcher (fn [item] (:type item)))
#'user/dispatcher
user=> (defmethod dispatcher "cat" cat-method
         [{:keys [lives]}]
         (/ lives 0))
#object[clojure.lang.MultiFn 0x6e1d4137 "clojure.lang.MultiFn@6e1d4137"]
user=> (dispatcher {:type "cat" :lives 9})
ArithmeticException Divide by zero  clojure.lang.Numbers.divide (Numbers.java:163)
user=> (pst *e 5)
ArithmeticException Divide by zero
	clojure.lang.Numbers.divide (Numbers.java:163)
	clojure.lang.Numbers.divide (Numbers.java:3813)
	user/eval155/cat-method--157 (NO_SOURCE_FILE:11)         ;; <== cat-method
	clojure.lang.MultiFn.invoke (MultiFn.java:229)
	user/eval161 (NO_SOURCE_FILE:12)

I think what would be best is if the defmethod function defaulted to a unique name based on the defmulti function name (if no name was specified).

Comment by Alex Miller [ 22/Oct/18 4:45 PM ]

CLJ-2212 is related

Comment by Marc O'Morain [ 23/Oct/18 9:30 AM ]

Oh, interesting.

The symbol that identifies the multimethod (dispatcher) in this case appended with some sort of string representation of the dispatch value would be a really nice default value here.

So you might have functions named
`dispatcher-default`, `dispatcher-cat`, etc.

I assume that you might have to add some additional uniqueness suffix on the end, too.





[CLJ-2418] [spec] (s/exercise string?) produces narrow string samples Created: 20/Oct/18  Updated: 20/Oct/18

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

Type: Enhancement Priority: Minor
Reporter: JAre Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: generative-test, generator, spec, string

Approval: Triaged

 Description   

(s/exercise string?) seems to never produce strings with delimiters or spaces. I noticed this while testing my string formatting function with generative testing - it was extremely surprising.

It looks like a potential source of low quality tests and bad surprises.






[CLJ-1103] Make conj assoc dissoc and transient versions handle args similarly Created: 04/Nov/12  Updated: 11/Oct/18

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

Type: Feature Priority: Minor
Reporter: Andy Fingerhut Assignee: Alex Miller
Resolution: Unresolved Votes: 10
Labels: collections, transient

Attachments: Text File clj-1103-10.patch     File