<< Back to previous view

[CLJ-2386] In ex-info, avoid recording stack frames of ex-info itself Created: 16/Aug/18  Updated: 17/Aug/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting

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

 Description   

ex-info is a function to construct ExceptionInfo instances. However, the construction captures the stack frames of calling ex-info, which are not what the caller intends to transmit. As a result, ex-info always has top stack frames that are about constructing the exception rather than the stack that is throwing:

user=> (defn f [] (throw (ex-info "oh no" {})))
#'user/f
user=> (f)
ExceptionInfo oh no  clojure.core/ex-info (core.clj:4739)
user=> (pst *e)
ExceptionInfo oh no {}
	clojure.core/ex-info (core.clj:4739)
	clojure.core/ex-info (core.clj:4739)
	user/f (NO_SOURCE_FILE:2)               <--- intended "top" of stack
	user/f (NO_SOURCE_FILE:2)
	user/eval148 (NO_SOURCE_FILE:3)
	user/eval148 (NO_SOURCE_FILE:3)
        ...

Proposed: Have ex-info construct the exception, but remove the ex-info stack frames. All callers then see the intended stack trace.

user=> (defn f [] (throw (ex-info "oh no" {})))
#'user/f
user=> (f)
ExceptionInfo oh no  user/f (NO_SOURCE_FILE:1)
user=> (pst *e)
ExceptionInfo oh no {}
	user/f (NO_SOURCE_FILE:1)
	user/f (NO_SOURCE_FILE:1)
	user/eval136 (NO_SOURCE_FILE:2)
	user/eval136 (NO_SOURCE_FILE:2)
	clojure.lang.Compiler.eval (Compiler.java:7069)
	clojure.lang.Compiler.eval (Compiler.java:7032)
        ...

Patch: clj-2386.patch

Screener Questions:

  1. elide-class-frames will turn a nil stack trace into an empty stack trace array. I don't think ex-info can ever have a nil stack trace, so maybe it doesn't matter.
  2. the elision is about both leading and class, and "class" is already known via the argument name, so maybe elide-leading-frames is a better name?





[CLJ-2389] [spec] Expose size parameter in clojure.spec.gen.alpha/generate Created: 17/Aug/18  Updated: 17/Aug/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: generator, spec
Environment:

Clojure 1.10.0-alpha6
clojure.spec.alpha 0.2.168


Approval: Triaged

 Description   

The lazy-loaded clojure.spec.gen.alpha/generate function only exposes one parameter of its delegate clojure.test.check.generators/generate:

(generate generator)

It should perhaps also accept the size parameter of clojure.test.check.generators/generate:

(generate generator size)

The default size 30 easily leads to OutOfMemoryError with complex specs.






[CLJ-2387] Port 65535 incorrectly excluded Created: 16/Aug/18  Updated: 16/Aug/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.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-2388] [spec] s/merge causes failure to roundtrip conform then unform Created: 16/Aug/18  Updated: 16/Aug/18

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

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

linux


Approval: Triaged

 Description   

Opened the bug after discussion in this thread: https://groups.google.com/forum/#!topic/clojure/r8WO24rHsi0

Essentially, when using s/merge with s/or, s/conform returns a structure that cannot be s/unform'ed.

Steps to reproduce (shamelessly copied from the thread):

(require '[clojure.spec.alpha :as s])
(s/def ::a (s/or :even even? :odd odd?))
(s/def ::b (s/or :even even? :odd odd?))
(s/def ::m1 (s/keys :req-un [::a]))
(s/def ::m2 (s/keys :req-un [::b]))
(s/def ::mm (s/merge ::m1 ::m2))
(s/valid? ::mm {:a 1 :b 2})      ;; true
(s/conform ::mm {:a 1 :b 2})
;;=> {:a 1, :b [:even 2]}
(s/unform ::mm {:a [:odd 1] :b [:even 2]})
;;=> {:a 1, :b [:even 2]}
(s/unform ::mm (s/conform ::mm {:a 1 :b 2}))





[CLJ-2373] improvements to exception messages and printing Created: 09/Jul/18  Updated: 16/Aug/18

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Unresolved Votes: 21
Labels: error-reporting, errormsgs, spec

Attachments: Text File clj-2373-2.patch     Text File clj-2373-3.patch     Text File clj-2373-4.patch     Text File clj-2373-clojure-1.patch     Text File clj-2373-spec-alpha-1.patch     Text File clj-2373-spec-alpha-2.patch    
Patch: Code
Approval: Vetted

 Description   

Problems

  • Discussions about "make errors better" often conflate error message, stacktraces, exception data
  • Errors do not clearly indicate in what "phase" of execution they occur (read, compile, macroexpand, evaluation, printing)
  • Errors during macroexpansion do not capture the location in caller source (like read and compile exceptions do), and thus the wrong "location" is reported
  • Clojure prints data into Throwable messages
    • big and ugly
    • outside user control

Principles

  • exception messages should be small strings
    • with the expectation that getMessage will be printed
    • totally controlled by throwing code
    • never print arbitrary data into a message
  • flow data all the way to the edge
    • ex-info & ex-data
    • macroexpand spec errors have a well known format
  • edge printing should be concise by default, configurable by user
    • edge functions like repl-caught respect print and spec bindings
    • concise summary by default
    • all the details when you want

Proposed impl changes

  • stop printing data into message strings
    • ExceptionInfo.toString should list keys, not entire map contents
    • spec.alpha/macroexpand-check should stop including explain-out in message
  • and instead print data (configurably) at the edges repl/pst and main/repl-caught
    • print spec per explain-out
    • print ExceptionInfo keys
  • make CompilerException wrappers special at print time instead of construct time
    • CE message is "CompilerException + file/line"
    • wrapped message is whatever it is
    • edge printers should print both

Discussion

  • what should tools do?
    • leverage the data provided in the exception chain to do whatever they want
    • can use main/repl-caught as a guide
  • what happens to programs that parse exception messages?
    • probably should not do this (Clojure's tests do this but try to minimize it)
    • this proposal changes some wrapper message text, but that was never part of the contract

Patch Status

  • clj-2373-spec-alpha-2.patch - for spec.alpha - don't print explain data into message (callers can do what they want with that)
  • clj-2373-4.patch - for clojure
    • LispReader - made ReaderException line/column fields public so CompilerEx can swipe them
    • Compiler
      • CompilerException now implements IExceptionInfo and works with ex-data
      • Created well-known keys :clojure.error/source,line,column,phase,symbol
      • Created well-known phases :read, :macroexpand, :spec (for macro), :compile. If no phase, then this is an unknown macroexpand error
      • Left existing CompilerException fields, but stored rest into data map
      • Construct new syntax error messages on construction
      • Use toString() to encapsulate combining wrapper and cause into a message
      • On read, spec, macroexpand exceptions, add appropriate wrapping
      • Tweaked existing compiler exception calls as much as possible to record symbol (more could potentially be done here - in some cases we have forms or non-symbol things we could report)
    • Var - expose a method to get the symbol for a var. Many people have requested this functionality via a core function, maybe this is a step in that direction.
    • clojure.main
      • add init-cause (private) function to get initial root (rather than existing weird root-cause)
      • add ex-str function (public) to construct the message to print based on an ex. tools could use this. Embeds some of the phase/printing logic but piggiebacks on CompilerException.toString() for some as well. Handles spec problem ex-data specially - this could be genericized but I have not tried to do that here.
      • change repl-caught to rely on ex-str
      • change main repl code to catch ReaderException and wrap appropriately (similar to what compiler does) and wrap print exceptions to handle that case
    • test* - in general these are tweaks to test to check the cause message rather than the wrapper message with the help of a new assertion type defined in test-helper

Also see: https://dev.clojure.org/display/design/Exception+handling+update



 Comments   
Comment by Stuart Halloway [ 09/Jul/18 12:14 PM ]

Output from a REPL session with the "-1" patches: https://gist.github.com/stuarthalloway/d3055d1975b464e8949b854e77a746b1

Comment by jcr [ 09/Jul/18 8:46 PM ]

re "many Clojure tests that parse exception strings need to change":

Is it desirable to decouple the error message (as printed to the user) from the error tag/type/kind (for tooling, tests, etc), and make the latter a part of the contract?

Comment by Alexander Yakushev [ 10/Jul/18 1:49 AM ]

I get the eliding map values from the output is meant to hide the ginormous Spec explanations. But printing only the keys doesn't have much value (pun intended). Is there possibly another solution? I can see two suboptimal ones:

1) Dissoc only the :clojure.spec.alpha/ keywords from the ex-data, Spec being the main offender.
2) Print ex-data with small print-length and print-depth (not sure whether that achieves anything, have to analyze different Spec errors to see if it does).

Comment by Stuart Halloway [ 10/Jul/18 6:21 AM ]

Thanks for the suggestions!

jcr: That might be desirable, but it is an expansion of scope and is not necessary to solve this problem.

Alexander: This is not just about spec errors, but all ex-data. The data is already present and accessible, and printing the keys gives the user a hint where to look.

Comment by Alex Miller [ 10/Jul/18 7:52 AM ]

I have dug into the test failures more and there are really two things here...

1) the top level exception messages have changed (and are generally worse for the particular case of a CompilerException with nested init exception, which is not in the example set). I think this bears revisiting for those cases where you might be in a "Java calling Clojure" environment.

2) the message printed to the repl has generally improved, however most of the failing tests are checking exception message as a proxy for what the user sees in a particular error situation via thrown-with-msg?. It would be helpful if "what is printed" could be more easily extracted and if the tests were actually checking that, rather than checking the exception message - I think this better reflects the tests' intent.

I'm working on an updated patch.

Comment by Alex Miller [ 12/Jul/18 12:12 AM ]

clj-2373-2.patch makes fewer changes in the CompilerException message (tests all continue to pass) but does more in the printing of exception chains in the repl, ignoring the computed message of the wrapper CompilerException completely and focusing more attention on the initial cause (but adding back in the source location info in the CompilerException wrapper if it's available).

user=> (/ 1 0)
ArithmeticException Divide by zero clojure.lang.Numbers.divide (Numbers.java:163)

user=> (throw (ex-info "oops" {:a 1 :b "foo"}))
ExceptionInfo oops clojure.core/ex-info (core.clj:4754)
ex-data keys: [:a :b]

user=> (let [a])
ExceptionInfo Call to clojure.core/let did not conform to spec., compiling:(3:1) clojure.core/ex-info (core.clj:4754)
In: [0] val: () fails spec: :clojure.core.specs.alpha/bindings at: [:args :bindings :init-expr] predicate: any?,  Insufficient input
Comment by Alex Miller [ 12/Jul/18 11:04 PM ]

Dumped some further design work at https://dev.clojure.org/display/design/Exception+handling+update

Still a WIP, take nothing here as final.

Comment by Alex Miller [ 11/Aug/18 4:11 PM ]

Pushed -3 patch for current state (does not address test failures yet).

Comment by Phill Wolf [ 12/Aug/18 11:26 AM ]

A vote for principles of brevity + data-to-the-edge. At the point of throw, code has no idea how many GB of stuff it's including in ex-data and no basis on which to prune it.





[CLJ-825] Protocol implementation inconsistencies when overloading arity Created: 08/Aug/11  Updated: 16/Aug/18

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

Type: Enhancement Priority: Major
Reporter: Carl Lerche Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: protocols
Environment:

All


Attachments: Text File clj-825-1.patch     File scribbles.clj    
Patch: Code and Test
Approval: Triaged

 Description   

The forms required for implementing arity-overloaded protocol methods are inconsistent between the "extend-*" macros and "defrecord".

The "extend" family of macros requires overloaded method definitions to follow the form used by defn:

(method ([arg1] ...) ([arg1 arg2] ...))

However, "defrecord" requires implementations to be defined separately:

(method [arg1] ...)
(method [arg1 arg2] ...)

Furthermore, the error modes if you get it wrong are unhelpful.

If you use the "defrecord" form with "extend-*", it evals successfully, but later definitions silently overwrite lexically previous definitions.

If you use the "extend-*" form with "defrecord", it gives a cryptic error about "unsupported binding form" on the body of the method.

This is not the same issue as CLJ-1056: That pertains to the syntax for declaring a protocol, this problem is with the syntax for implementing a protocol.

(defprotocol MyProtocol
  (mymethod
    [this arg]
    [this arg optional-arg]))

(extend-protocol MyProtocol
  Object
  (mymethod
    ([this arg] :one-arg)
    ([this arg optional-arg] :two-args)))

;; BAD! Blows up with "Unsupported binding form: :one-arg"
(defrecord MyRecord []
  MyProtocol
  (mymethod
    ([this arg] :one-arg)
    ([this arg optional-arg] :two-args)))

;; Works...
(defrecord MyRecord []
  MyProtocol
  (mymethod [this arg] :one-arg)
  (mymethod [this arg optional-arg] :two-args))

;; Evals...
(extend-protocol MyProtocol
  Object
  (mymethod [this arg] :one-arg)
  (mymethod [this arg optional-arg] :two-args))

;; But then... Error! "Wrong number of args"
(mymethod :obj :arg)

;; 2-arg version is invokable...
(mymethod :obj :arg1 :arg2)


 Comments   
Comment by Paavo Parkkinen [ 17/Nov/13 6:02 AM ]

Attached a patch for this.

For defrecord, I check which style is used for defining methods, and transform into the original style if the new style is used. For the check I do what I believe defn does, which is (vector? (first fdecl)).

For extend-*, I skip the checking, and just transform everything into the same format.

Tests included for both.

All tests pass.

Comment by Rich Hickey [ 10/Jun/14 11:00 AM ]

What the proposal?

Comment by Vladimir Tsanev [ 16/Aug/18 9:15 AM ]

This hit me too.

I workaround this using extend for the protocols that have multi arity methods
i.e. for the above example

(extend Object
  MyProtocol
  {:mymethod (fn ([^Object this arg] :one-arg)
                 ([^Object this arg optional-arg] :two-args))})




[CLJ-2287] Add clojure.set/union, intersection, difference to core.spec.alpha Created: 12/Dec/17  Updated: 14/Aug/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





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

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

Type: Defect Priority: Minor
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: android
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.patch    
Patch: Code
Approval: Vetted

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



 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.





[CLJ-1656] Unroll assoc and assoc! for small numbers of arguments Created: 06/Feb/15  Updated: 13/Aug/18

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

Type: Enhancement Priority: Major
Reporter: Tom Crayford Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: performance

Attachments: File assoc.diff     Text File assoc-gen-test.patch     Text File CLJ-1656-v1.patch     Text File CLJ-1656-v2.patch     Text File CLJ-1656-v3.patch     Text File CLJ-1656-v4.patch     Text File CLJ-1656-v5.patch     Text File CLJ-1656-v6.patch     Text File CLJ-1656-v7.patch     Text File CLJ-1656-v8.patch     File cpuinfo     File javaversion     File output     File uname    
Patch: Code and Test
Approval: Triaged

 Description   

Whilst doing performance work recently, I discovered that unrolling to single assoc calls were significantly faster than using multiple keys (~10% for my particular application). Zachary Tellman then pointed out that clojure.core doesn't unroll assoc at all, even for the case of relatively low numbers of keys.

We already unroll other performance critical functions that call things via `apply`, e.g. `update` https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L5914, but `assoc` (which is, I think in the critical path for quite a bunch of applications and libraries), would likely benefit from this.

I have not yet developed patches for this, but I did some standalone benchmarking work:

https://github.com/yeller/unrolling-assoc-benchmarks

benchmark results:

code: https://github.com/yeller/unrolling-assoc-benchmarks/blob/master/src/bench_assoc_unrolling.clj

  1 2 3 4
empty array map (not unrolled) 23ns 93ns 156ns 224ns
empty array map (unrolled assoc) N/A 51ns 80ns 110ns
         
20 element persistent hashmap (not unrolled) 190ns 313ns 551ns 651ns
20 element persistent hashmap (unrolled assoc) N/A 250ns 433ns 524ns
         
record (not unrolled) 12ns 72ns 105ns 182ns
record (unrolled assoc) N/A 21ns 28ns 41ns

Each measurement was made in a separate JVM, to avoid JIT path dependence.

Benchmarks were ran on a commodity server (8 cpus, 32gb ram), with ubuntu 12.04 and a recent release of Java 8. Attached are `cpuinfo`, `uname` and `java -version` output.

Relatively standard JVM production flags were enabled, and care was taken to disable leiningen's startup time optimizations (which disable many of the JIT optimizations).

Benchmarks can be run by cloning the repository, and running `script/bench`

There's one outstanding question for this patch: How far should we unroll these calls? `update` (which is unrolled in the 1.7 alphas) is unrolled to 3 arguments. Adding more unrolling isn't difficult, but it does impact the readability of assoc.

Patch: CLJ-1656-v5.patch



 Comments   
Comment by Tom Crayford [ 09/Feb/15 12:01 PM ]

Ok, attached `assoc.diff`, which unrolls this to a single level more than the current code (so supporting two key/value pairs without recursion). The code's going to get pretty complex in the case with more than the unrolled number of keys if we continue on this way, so I'm unsure if this is a good approach, but the performance benefits seem very compelling.

Comment by Michael Blume [ 09/Feb/15 3:35 PM ]

Since the unroll comes out kind of hairy, why not have a macro write it for us?

Comment by Michael Blume [ 09/Feb/15 4:03 PM ]

Patch v2 includes assoc!

Comment by Tom Crayford [ 09/Feb/15 5:01 PM ]

I benchmarked conj with similar unrolling, across a relatively wide range of datatypes from core (lists, sets, vectors, each one empty and then again with 20 elements):

  1 2 3 4
empty vector (not unrolled) 19ns 57ns 114ns 126ns
empty vector (unrolled conj) N/A 44ns 67ns 91ns
         
20 element vector (not unrolled) 27.35ns 69ns 111ns 107ns
20 element vector (unrolled conj) N/A 54ns 79ns 104ns
         
empty list (not unrolled) 7ns 28ns 53ns 51ns
empty list (unrolled conj) N/A 15ns 20ns 26ns
         
twenty element list (not unrolled) 8.9ns 26ns 49ns 49ns
twenty element list (unrolled) N/A 15ns 19ns 30ns
         
empty set (not unrolled) 64ns 170ns 286ns 290ns
empty set (unrolled) N/A 154ns 249ns 350ns
         
twenty element set (not unrolled) 33ns 81ns 132ns 130ns
twenty element set (unrolled) N/A 69ns 108ns 139ns

Benchmarks were run on the same machine as before. There's a less clear benefit here, except for lists, where the overhead of creating seqs and recursing seems to be clearly dominating the cost of actually doing the conj (which makes sense - conj on any element list should be a very cheap operation). Raw benchmark output is here: https://gist.github.com/tcrayford/51a3cd24b8b0a8b7fd74

Comment by Tom Crayford [ 09/Feb/15 5:04 PM ]

Michael Blume: I like those patches! They read far nicer to me than my original patch. Did you check if any of those macro generated methods blew Hotspot's hot code inlining limit? (it's 235 bytecodes). That'd be my only worry with using macros here - it's easy to generate code that defeats the inliner.

Comment by Michael Blume [ 09/Feb/15 5:57 PM ]

Thanks! This is new for me, so I might be doing the wrong thing, but I just ran nodisassemble over both definitions and the "instruction numbers" next to each line go up to 219 for the varargs arity assoc and up to 251 for assoc!, so, assuming I'm looking at the right thing, maybe that one needs to have an arity taken off? If I remove the highest arity I get 232 for varargs which is just under the line.

I guess alternatively we could call assoc! instead of assoc!* in the varargs arity, which removes a lot of code – in that case it's 176 for varargs and 149 for six pairs.

Comment by Michael Blume [ 09/Feb/15 6:01 PM ]

Gah, I forgot to include coll in the varargs call to assoc!

which reminds me that this patch needs tests.

Comment by Michael Blume [ 09/Feb/15 10:27 PM ]

OK, this has some fixes I made after examining the disassembled output. There's a change to the assoc!* macro to be sure it type-hints correctly – I'm honestly not sure why it didn't type-hint properly before, but it does now. Also, I changed the call to assoc! rolling up the first six entries at the top of the varargs version from a macro call to a function call so it'd fit within the 251 inlineable bytecodes. (This, again, is assuming I'm reading the output correctly).

Comment by Tom Crayford [ 10/Feb/15 6:38 AM ]

Michael: Wanna push a branch with these patches to clojars or something? Then I can rerun the benchmarks with the exact code in the patches.

Comment by Michael Blume [ 10/Feb/15 2:36 PM ]

Hmm, not sure I know how to do that – here's a branch on github though https://github.com/MichaelBlume/clojure/tree/unroll-assoc

Comment by Michael Blume [ 12/Feb/15 1:12 PM ]

v5 marks the helper macros private.

Comment by Tom Crayford [ 13/Feb/15 4:11 AM ]

Michael: was that branch just based off clojure/clojure master? I tried running benchmarks off it, but ran into undefined var errors when building this code (which doesn't happen with alpha5):

(Retrieving com/yellerapp/clojure-unrolled-assoc/1.7.0-unrollassoc-SNAPSHOT/clojure-unrolled-assoc-1.7.0-unrollassoc-20150213.092242-1.pom from clojars)
(Retrieving com/yellerapp/clojure-unrolled-assoc/1.7.0-unrollassoc-SNAPSHOT/clojure-unrolled-assoc-1.7.0-unrollassoc-20150213.092242-1.jar from clojars)
(Retrieving org/clojure/clojure/1.3.0/clojure-1.3.0.jar from central)
Exception in thread "main" java.lang.RuntimeException: Unable to resolve symbol: bench in this context, compiling:(bench_assoc_unrolling.clj:5)
at clojure.lang.Compiler.analyze(Compiler.java:6235)
at clojure.lang.Compiler.analyze(Compiler.java:6177)
at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3452)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6411)
at clojure.lang.Compiler.analyze(Compiler.java:6216)
at clojure.lang.Compiler.analyze(Compiler.java:6177)
at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5572)
at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5008)

Comment by Michael Blume [ 23/Feb/15 5:08 PM ]

Ok, how are you building? Why the strange clojure group?

Comment by Michael Blume [ 23/Feb/15 5:09 PM ]

The existing version of assoc does runtime checking that an even number of varargs are passed in, but assoc! does not. Do we want to preserve this behavior or do checks in both?

Comment by Michael Blume [ 23/Feb/15 6:00 PM ]

Also, I'm curious how relevant inlining is here – does HotSpot inlining actually work with Var invocation when there's a getRootBinding step in the way?

Comment by Alex Miller [ 23/Feb/15 7:59 PM ]

Yes, inlining works through var invocation.

Comment by Tom Crayford [ 16/Mar/15 7:05 AM ]

Michael,

That group is just an uploaded version of clojure master with your patches applied, built in just the same way as before (you should be able to check out the repo and replicate).

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

The patch CLJ-1656-v5.patch doesn't seem to do anything with the old version of assoc (in core.clj around line 179)?

The new one needs to have the arglists and other stuff like that. I'm not sure about the macro/private vars in there either. Did you try leveraging RT.assocN() with a vector?

Are there existing tests in the test suite for assoc with N pairs?

Comment by Michael Blume [ 29/Apr/15 8:46 PM ]

The dependencies in clojure.core were such that assoc needed to be defined well before syntax-quoting, so I just let it be defined twice, once slower, once faster. I'll put up a patch with arglists. Does it need an arglist for every new arity, or are the existing arglists enough? (I'm afraid I'm not 100% solid on what the arglists metadata does) There is an annoying lack of existing tests of assoc. I have a generative test in my tree because that seemed more fun than writing cases for all the different arities. I can post it if it seems useful, it might be overkill though.

Comment by Michael Blume [ 29/Apr/15 9:50 PM ]

Here's the test patch I mentioned, it's even more overkill than I remembered

Comment by Michael Blume [ 29/Apr/15 10:01 PM ]

There, code and test.

This also checks that assoc! is passed an even number of kvs in the varargs case, which is the behavior of assoc. The test verifies that both assoc and assoc! throw for odd arg count.

Comment by Alex Miller [ 29/Apr/15 11:10 PM ]

The existing arglist is fine - it just overrides the generated one for doc purposes.

Did you try any of the RT.assocN() stuff?

I guess another question I have is whether people actually do this enough that it matters?

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

Updated patch to apply to master

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

This still needs assessment on frequency of different counts. Based on some very quick checking, passing two kvs is common enough to matter. Passing three is far less common and so on. But I'd love to see some rough idea of frequency from running some stuff. I think the current unrolling is too much (2-3 kvs is probably sufficient).





[CLJ-2385] graal native-image complains about tap-loop thread Created: 11/Aug/18  Updated: 11/Aug/18

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

Type: Enhancement Priority: Minor
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File delay_tap_loop.patch    
Patch: Code

 Description   

Not the greatest rationale: Graal native-image complains that the tap-loop thread starts during class-loading. It is possible to delay starting it until the first tap registers, which is exactly what the attached patch does.






[CLJ-2380] read/print handlers for inst/java.util.Date inconsistent between different JDKs Created: 03/Aug/18  Updated: 10/Aug/18

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

Type: Enhancement Priority: Minor
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File smaller_conditional.patch    
Patch: Code

 Description   

Problem
The default REPL printing and reading wrt Dates is inconsistent between a minimal runtime environment (like a JVM with only the java.base module, or the deprecated JDK8 bootclasspath/bcp) and a full JVM.

There are a handful of references in Clojure to classes that live outside of the java.base module (or bcp). In such all places the referent class does not load automatically during Clojure's init. (They are loaded on demand, e.g. when the user first calls bean) The load of the core.instant namespace became conditional in Clojure 1.9, depending on the availability of the java.sql.Timestamp class. This was an accommodation made to enable lein to Clojure to run on the Java 8 bootclasspath, which lein used as a hack to avoid bytecode verification and thus load faster. (The reference to java.sql.Timestamp was verboten on the bcp) This also had the by-product of not loading read/print handlers for java.util.Date, which do exist on the bcp or java.base module.

present situation

  • lein has meanwhile stopped (mis)using the bootclasspath (instead skipping bytecode verification explicitly)
  • The notion of a bootclasspath no longer exists as of JDK9
  • There is now a bottom module with a clearly-defined set of classes, including java.util.Date and java.time.Instant
  • jlinking JVMs is a common and effective container deployment strategy

Clojure already runs fine on the bootclasspath or a JVM with only #{java.base}, but because the default data reader & clojure.instant aren't loaded, no read/print handlers for inst/j.u.Date get installed. The conditional load is about java.sql.Timestamp, which is never in default use anyways (besides the print handler) but it affects things that work fine in a reduced environment. (java.sql.Timestamp is in the java.sql module, which adds an additional 12MB to a jlinked jvm)

Great tagged value handling for dates sets Clojure's REPL apart from other dynlangs. It would be nice to have those conveniences available consistently on all modularized JDKs.

considerations
reduce the scope of the conditional load to avoid affecting date read/print handlers?
move the install of the read handler into clojure.instant, and load it unconditionally? (There was never a promise about when the handler was installed)
java.time.Instant is also in the bootclasspath or java.base module

informational
The following is a list of class references outside of the java.base module, not including the check for java.sql.Timestamp inside core.clj

non java.base references
clojure.core$bean                                  -> java.beans.BeanInfo                                java.desktop
   clojure.core$bean$fn__6987$fn__6988                -> java.beans.PropertyDescriptor                      java.desktop
   clojure.core$resultset_seq                         -> java.sql.ResultSet                                 java.sql
   clojure.inspector$inspect                          -> java.awt.BorderLayout                              java.desktop
   clojure.inspector$inspect_table                    -> java.awt.Component                                 java.desktop
   clojure.inspector$inspect_tree                     -> java.awt.Component                                 java.desktop
   clojure.instant$construct_timestamp                -> java.sql.Timestamp                                 java.sql
   clojure.instant$print_timestamp                    -> java.sql.Timestamp                                 java.sql
   clojure.java.browse_ui$open_url_in_swing           -> java.awt.Component                                 java.desktop
   clojure.lang.XMLHandler                            -> org.xml.sax.Attributes                             java.xml
   clojure.repl$set_break_handler_BANG_               -> sun.misc.Signal                                    JDK internal API (jdk.unsupported)
   clojure.repl$set_break_handler_BANG_               -> sun.misc.SignalHandler                             JDK internal API (jdk.unsupported)
   clojure.repl.proxy$java.lang.Object$SignalHandler$d8c00ec7 -> sun.misc.Signal                                    JDK internal API (jdk.unsupported)
   clojure.repl.proxy$java.lang.Object$SignalHandler$d8c00ec7 -> sun.misc.SignalHandler                             JDK internal API (jdk.unsupported)
   clojure.xml$startparse_sax                         -> javax.xml.parsers.SAXParserFactory                 java.xml
   clojure.xml$fn__8900                               -> org.xml.sax.ContentHandler                         java.xml


 Comments   
Comment by Ghadi Shayban [ 10/Aug/18 3:29 PM ]

approach in the patch

  • load clojure.instant unconditionally, as well as the #inst reader
  • wrap only the sql.Timestamp print-method extensions with a conditional load
  • remove when-class (now unused, it will not work for wrapping the defmethods because the java.sql.Timestamp class constants need to be delayed)




[CLJ-2384] Add support for ~/ directive in cl-format Created: 09/Aug/18  Updated: 09/Aug/18

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

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


 Description   

clojure.pprint/cl-format does not support the ~/ directive which allows to extend the formatter with custom functions.

The doc at https://clojure.github.io/clojure/doc/clojure/pprint/CommonLispFormat.html mentions that support for ~/ is next up.

This Jira is to ask if someone is willing to contribute that enhancement to it.






[CLJ-2383] Add classes that were introduced to java.lang in Java 7/8 into DEFAULT_IMPORTS Created: 08/Aug/18  Updated: 08/Aug/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

Approval: Triaged

 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


 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.





[CLJ-2365] Consider integration with java.util.function interfaces Created: 26/Jun/18  Updated: 07/Aug/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: interop

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.)





[CLJ-2381] [spec] ::defn-args spec does not capture type hint on return value Created: 06/Aug/18  Updated: 07/Aug/18

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

Type: Defect Priority: Major
Reporter: Mark Engelberg Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

Windows, Java 1.8


Approval: Triaged

 Description   

Conforming a defn to the ::defn-args spec correctly preserves any type hint given to the inputs, but incorrectly loses the type hint given to the return value.

user=> (require '[clojure.spec.alpha :as s] '[clojure.core.specs.alpha :as ss])
nil
user=> (def c '(f ^double [^long x] 0.0))
#'user/c
user=> (binding [*print-meta* true] (prn c))
^{:line 10, :column 9} (f ^double [^long x] 0.0)
nil
user=> (binding [*print-meta* true] (prn (s/conform ::ss/defn-args c)))
{:name f, :bs [:arity-1 {:args {:args [[:sym ^long x]]}, :body [:body [0.0]]}]}
nil

Cause: Conformed regex specs do not retain meta on their conformed value (coll specs do):

user=> (binding [*print-meta* true] (prn (s/conform (s/coll-of int?) ^{:hi :there} [1])))
^{:hi :there} [1]

user=> (binding [*print-meta* true] (prn (s/conform (s/* int?) ^{:hi :there} [1])))
[1]

The particular spec where this comes up here is :clojure.core.specs.alpha/arg-list, which is an s/*.






[CLJ-2382] Unnecessary reflection with `indexOf` on vector Created: 07/Aug/18  Updated: 07/Aug/18

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

Type: Defect Priority: Minor
Reporter: Tianxiang Xiong Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop, reflection


 Description   

This generates a reflection warning:

(let [id-1 3
      id-2 1
      id-3 2
      ids [id-1 id-2 id-3]]
  #(compare (.indexOf ids %1) (.indexOf ids %2)))
;; => call to method indexOf on clojure.lang.IPersistentVector can't be resolved (no such method).

This does not:

(let [ids [3 2 1]]
  #(compare (.indexOf ids %1) (.indexOf ids %2)))

The only difference is whether the elements of the vector are literal, which shouldn't affect the call to indexOf.



 Comments   
Comment by Alex Miller [ 07/Aug/18 12:51 PM ]

I believe what's happening here is that in the first case, it gets typed as a PersistentVector (concrete data structure read by the reader). PV implements Collection and so requires no reflection.

In the second case, the vector requires evaluation and its type is IPersistentVector (the interface). IPV does not extend Collection (its independent of that interface) and so it can't find the indexOf method.

Stepping back from both, we're doing Java interop on an object that we are expecting to be a Collection. I'm somewhat on the fence about whether we should expect ids to carry this info when performing interop on it here. Depends how much you think that the abstract Clojure vector collection is a Java collection (vs the implementation of one).

Doesn't seem easily "fixable" to me in ways that are clean.





[CLJ-2034] comment macro doesn't ignore namespace keyword Created: 04/Oct/16  Updated: 06/Aug/18  Resolved: 06/Aug/18

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

Type: Defect Priority: Minor
Reporter: Nuttanart Pornprasitsakul Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

In fresh started REPL:

user=> (comment (x/y))
nil
user=> (comment ::x/y)
RuntimeException Invalid token: ::x/y  clojure.lang.Util.runtimeException (Util.java:221)
RuntimeException Unmatched delimiter: )  clojure.lang.Util.runtimeException (Util.java:221)


 Comments   
Comment by Alex Miller [ 04/Oct/16 11:37 PM ]

This code is not valid because x is an invalid alias (not bound to anything).

This works for example:

user=> (alias 'x 'clojure.string)
nil
user=> (comment ::x/y)
nil

comment still reads the code, so code that is unable to be read is still invalid.

Comment by Alex Miller [ 04/Oct/16 11:38 PM ]

Note that CLJ-2030 would make this code work (by auto-creating x as a new namespace).





[CLJ-714] Real multi-line comments Created: 11/Jan/11  Updated: 06/Aug/18  Resolved: 06/Aug/18

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

Type: Enhancement Priority: Minor
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Declined Votes: 5
Labels: None

Attachments: File 0001-Add-multi-line-comments.diff     Text File patch.txt    
Patch: Code and Test

 Description   

The (comment) macro is not a viable substitute for real multi-line comments:

  • It evaluates to nil, so isn't really useful except at the top level
  • It requires proper paren/etc nesting inside itself
  • Contents must be valid tokens; for example TODO: is illegal inside a (comment)

Common Lisp has #|...|# for multi-line comments, and I think Clojure would benefit from having them too. I've implemented #| to behave in a way that is identical (so far as I am able to test) to CL's implementation, and added tests to the reader test to verify that they work.

Apologies for my formatting in the java files not quite matching up with the rest of the code: I didn't want to go through the pain of making it perfect unless this patch would actually be accepted; I can go back through if necessary.



 Comments   
Comment by Anthony Simpson [ 11/Jan/11 11:56 PM ]

I am totally for this. I agree that comment isn't a valid substitution. comment is useless for a lot of things.

Comment by Benjamin Teuber [ 13/Jan/11 6:09 AM ]

Nice - but are you sure just doing nothing on an unexpected EOF is a good idea?
I guess it usually means I've forgotten the close tag somewhere, so I'd rather be informed about it by an exception. Omitting the |# on purpose just to save two characters seems dirty to me.

Comment by G. Ralph Kuntz, MD [ 13/Jan/11 8:19 AM ]

What about #_(...)?

I know that the parens have to be balanced, but other than that, doesn't it do everything else?

Comment by Anthony Simpson [ 13/Jan/11 8:35 AM ]

No.

user=> #_(TODO: Hi there!)
java.lang.Exception: Invalid token: TODO:

And then, as you mentioned, we have the paren balance problem. The idea is to have something completely ignores what is inside of it.

Comment by Alan Malloy [ 13/Jan/11 1:13 PM ]

Good point, Benjamin. I just assumed most languages didn't need comments to close nicely, but having tried it out I can't find a single one that would let me get away with an unclosed multi-line comment.

Revised patch attached, and using git format-patch now that I know it exists.

Comment by a_strange_guy [ 13/Jan/11 4:02 PM ]

You can use #_ with string literals already:

#_"TODO: Hi there!"

The only problem we have is that we don't have raw strings. I don't think that we need another litrals for comments.

And I don't see any use for multiline comments. In Java etc. you use them to uncomment parts of your program (we have #_ (...) for that) or to document classes & methods (we have docstrings which are superior). If you need multiline comments to explain some part of your code, then you should do something about the code (or use #_ "..." if you are lazy).

Comment by Anthony Simpson [ 13/Jan/11 4:20 PM ]

Except with #_"" you can't just paste something in there because it has to be escaped properly if it has strings and such embedded in it.

Sure, we can work around these limitations somewhat. We can pile hacks on hacks to get half-working multiline comments or just add this trivial patch and have real multiline comments like every other language in existence. Just sayin'.

Multi-line comments aren't for just explaining bad code. #_ isn't designed for this and thus is not a solution to this, and marking every line of a prewritten text with ; and/or removing or editing it later is just tedious.

Curiosity: wouldn't this be useful for literate programming tools?

Comment by Stuart Halloway [ 14/Jan/11 7:18 AM ]

Rich: if you are interested in this assign to me and I will screen.

Comment by a_strange_guy [ 14/Jan/11 11:41 PM ]

Don't think that this is a pressing issue, because every editor that has support for Clojure (or even just Scheme/CL) has a comment-region feature.

I would support adding a reader macro for raw strings, because that is an issue (almost every ns docstring is escaped), and if we had those, then multiline comments would be subsumed by them:

#_
#[[ ;; lua-like proposal for raw strings

]]
Comment by Lau Jensen [ 15/Jan/11 7:26 AM ]

If its a pressing issue or not isn't really the question is it?

To me its a straightforward patch which adds functionality currently not found in Clojure and which I personally would label a welcomed addition.

Comment by Chouser [ 24/Jan/11 9:40 AM ]

Sure it adds a feature, but it also adds syntax and complexity to the language. If the feature it adds doesn't solve real problems of sufficient importance, it may be a net loss for the language.

For temporarily commenting out code, #_ and (comment ...) work fine. For hunks of documentation text, ; or ;; work nicely, especially if you have an ultra-advanced editor like vim to handle your text reformatting. Is there another use case?

Comment by Fogus [ 24/Jan/11 9:59 AM ]

I'm with Chouser on this. Additionally, I would add that the description of #_ is "The form following #_ is completely skipped by the reader." However, trying something like:

    (defn gimmie-pi []
      #_( TODO: validate the accuracy
                of the return value. )
      3)

currently throws an exception. Maybe the form should really be skipped by the reader and allow free form text? I don't know if this is the right answer, but I would feel more comfortable with that than a new reader macro.

Comment by Alan Malloy [ 24/Jan/11 11:21 AM ]

I don't think that's possible, Fogus. If it's going to skip "a form", then it has to understand the syntax therein well enough to determine which closing paren matches up.

Comment by Fogus [ 24/Jan/11 11:40 AM ]

> which closing paren matches up

I'm sorry, I was not clear that I didn't expect my idea to provide the entire functionality of the proposed #| ... |#.

I don't see the utility in providing a whole new reader macro to allow mismatched embedded parentheses. I can kinda see the value in allowing things like `TODO:`, although I would probably just use #_() or comment and just make sure that my embedded comment was not malformed and ; otherwise.

Comment by Rich Hickey [ 26/Jan/11 7:12 AM ]

I'm opposed. I don't think the benefit/complexity ratio (especially for tools) is high enough. We've done without for quite a while and no one's caught fire.

Comment by Chas Emerick [ 03/Sep/11 10:05 PM ]

Closed per request from Alan Malloy.





[CLJ-1146] Symbol name starting with digits to defn throws "Unmatched delimiter )" Created: 13/Jan/13  Updated: 06/Aug/18

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

Type: Enhancement Priority: Trivial
Reporter: Linus Ericsson Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs, reader
Environment:

$java -jar clojure-1.5.0-RC2.jar

$java -version
java version "1.6.0_37"
Java(TM) SE Runtime Environment (build 1.6.0_37-b06-434-10M3909)
Java HotSpot(TM) 64-Bit Server VM (build 20.12-b01-434, mixed mode)
Mac OS X:
System Version: Mac OS X 10.6.8 (10K549)
Kernel Version: Darwin 10.8.0



 Description   

When trying to use an invalid symbol name when defining a function, the error message thrown is a confusing and wrong one. The error message is "RuntimeException Unmatched delimiter: ) clojure.lang.Util.runtimeException (Util.java:219)", which unfortunately is the only message seen in nrepled emacs.

$ java -jar clojure-1.5.0-RC2.jar
Clojure 1.5.0-RC2
user=> (defn 45fn [] nil)
NumberFormatException Invalid number: 45fn clojure.lang.LispReader.readNumber (LispReader.java:255)
[]
nil
RuntimeException Unmatched delimiter: ) clojure.lang.Util.runtimeException (Util.java:219)

Expected:
When trying to (defn or (def a thing with a non valid symbol name, the last thrown error message should be one stating that the given symbol name is not a valid one.



 Comments   
Comment by Kevin Downey [ 18/Apr/14 2:27 AM ]

this is an artifact of how streams and repls work.

when you type (defn 45fn [] nil) and hit enter, the inputstream flushes and "(defn 45fn [] nil)" is made available to the reader, the reader reads up to 45fn, throws an error back to the main repl loop, which prints out the error, then calls read, which still has the unread parts available to it "[] nil)"

changing this behavior would require significant changes to clojure's repl.

checkout https://github.com/trptcolin/reply instead

Comment by Tianxiang Xiong [ 06/Feb/18 3:02 AM ]

Why does reader continue to read after encountering an exception? Shouldn't it immediately stop?

If someone could point me to the relevant code, that'd be helpful.

Comment by Andy Fingerhut [ 06/Feb/18 3:24 AM ]

To "immediately stop" in a REPL, do you mean stop reading anything more in the REPL session at all? If you did that, it would just 'freeze up' the REPL session. I don't think anyone wants that behavior.

The reader doesn't continue to read after encountering an exception. It raises the exception. The L (loop) part of the REPL goes back to read again, starting at the last part not read yet, which in the example is "[] nil )", as Kevin Downey explained in his comment.

Perhaps you could imagine a different behavior something like: if the reader raises an exception, then before the REPL goes back to read again, it should discard the rest of the input line it was on if an exception was raised. I don't know whether such a thing is a change desired by the Clojure core team in the standard REPL. You can write your own REPLs that do that, of course.

See the function 'repl' in the clojure.main namespace. At a REPL, you can see it by evaluating: (source clojure.main/repl)

Comment by Andy Fingerhut [ 06/Feb/18 3:27 AM ]

Addendum: Stuart Halloway's talk on REPL-Driven Development has some notes on customizing the built-in REPL, or writing your own. See: https://github.com/matthiasn/talk-transcripts/blob/master/Halloway_Stuart/REPLDrivenDevelopment.md

Comment by Tianxiang Xiong [ 06/Feb/18 4:37 AM ]

if the reader raises an exception, then before the REPL goes back to read again, it should discard the rest of the input line it was on if an exception was raised.

That's certainly what I'd expect--but it shouldn't discard the rest of the input line, rather the entirety of the remaining input.

The way the REPL currently works means it does not set *e to the first exception that caused it to fail. The only indication the user has of the "real" error is a printed message (the result of (catch e)). They (or rather their tooling) aren't able to analyze the right error.

What's more, the current behavior could lead to some surprising (and potentially destructive) results.

For example,

=> (do {:foo} (println "this shouldn't happen") 
RuntimeException Map literal must contain an even number of forms  clojure.lang.Util.runtimeException (Util.java:221)
this shouldn't happen
nil

This example is trivial, but imagine that the println was some destructive operation dependent on the validity of the preceding form.

Comment by Tianxiang Xiong [ 06/Feb/18 6:13 AM ]

The following solves the problem:

(defn flush-input
  "Flush input reader, default *in*."
  ([]
   (flush-input *in*))
  ([in]
   (loop []
     (when (.ready in)
       (.read in)
       (recur)))))

(defn repl-caught
  "Default :caught hook for repl"
  [e]
  (let [ex (repl-exception e)
        tr (.getStackTrace ex)
        el (when-not (zero? (count tr)) (aget tr 0))]
    (flush-input)    ;; new
    (binding [*out* *err*]
      (println (str (-> ex class .getSimpleName)
                    " " (.getMessage ex) " "
                    (when-not (instance? clojure.lang.Compiler$CompilerException ex)
                      (str " " (if el (stack-element-str el) "[trace missing]"))))))))

R.e. the original issue:

=>  (defn 45fn [] nil)
NumberFormatException Invalid number: 45fn  clojure.lang.LispReader.readNumber (LispReader.java:332)

=> *e

#error {
 :cause "Invalid number: 45fn"
 :via
...
Comment by Andy Fingerhut [ 06/Feb/18 10:30 AM ]

A caution here: "Discard the rest of the input" might be a nondeterministic approach in some scenarios. I haven't tested this, but suppose I copy and paste a block of 20 lines of code into the REPL, and the reader raises an exception somewhere during the first line. "The rest of the input" at that instant might be the rest of the first line, or half of the 20 lines, or all of it. It may depend upon how data was broken up across TCP packets in a socket REPL, or the timing of how your terminal decides to present the characters to the Clojure process.

Comment by Tianxiang Xiong [ 06/Feb/18 12:25 PM ]

Don't think that'd be a worse outcome than currently. In both cases the remainder of the bad input is evaluated. And I'm not sure how much of a problem this would be--most data entered into the REPL is a few lines at most.

Comment by Andy Fingerhut [ 06/Feb/18 1:10 PM ]

A difference between the way it is now, and your proposal, if I am correct in my guess that the 'discard the rest of the input' approach is nondeterminstic, is that non-determinism that you are introducing. I could copy and paste the same 20 lines in the current REPL, and always get the same exceptions (given the same starting state, if that mattered).

I could copy and paste the same 20 lines in your modified REPL, and sometimes get different behavior.

If someone filed a bug for that non-determinism, and you were responsible for responding to it, how would you respond?

Comment by Tianxiang Xiong [ 06/Feb/18 2:39 PM ]

I'd say fix the code you're sending to the REPL, b/c an exception has been raised. Then enter it into the REPL again after you've fixed it. Note that flushing the input does not occur unless an exception is raised.

Comment by Andy Fingerhut [ 06/Feb/18 4:29 PM ]

Sure, but you can fix the code you're sending to the REPL, because an exception has been raised, with the code as it is today, too.

The flushing change would mean - without that change, reader exceptions result in a predictable point in the read stream where reading starts again. With the flushing change, reader exceptions result in an unpredictable point in the read stream where reading starts again.

Comment by Tianxiang Xiong [ 06/Feb/18 7:53 PM ]

you can fix the code you're sending to the REPL, because an exception has been raised, with the code as it is today, too.

The point is that it's harder to do so because the final error reported is not the first error from the read failure. Which error message would you rather see for OP's problem: NumberFormatException Invalid number: 45fn or RuntimeException Unmatched delimiter: )? And yes, the Invalid number ... exception will be printed by the REPL, but it won't be set as *e, which is the reified error available to tooling.

I don't know how big a problem the incomplete flushing you mention is. I'd surmise that for most input, it's not a problem. It hasn't been a problem for my locally modified version of Clojure. I know nothing about the socket REPL, so I've no idea if it'd be more common there.

To completely solve this problem the REPL would need to have some idea of what a "complete" input is--i.e. everything entered before the user hit RET. If someone has any pointers on how that could be done, I'd appreciate it. Otherwise I'd say we shouldn't rule out a 99% solution over a 1% edge case.





[CLJ-1473] Badly formed pre/post conditions silently passed Created: 24/Jul/14  Updated: 03/Aug/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: 5
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.





[CLJ-2379] `proxy` fails with redefined interfaces Created: 02/Aug/18  Updated: 02/Aug/18

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop

Attachments: Text File 0001-CLJ-2379-idempotent-proxy-name-just-on-identical-ins.patch     Text File 0001-CLJ-2379-idempotent-proxy-name-just-on-identical-ins-v2.patch    
Patch: Code
Approval: Triaged

 Description   

Before patch:

user=> (definterface I (f []))
user.I
user=> (def p (proxy [Object I] [] (f [] 1)))
#'user/p
user=> (definterface I (f []))
user.I
user=> (def p (proxy [Object I] [] (f [] 1)))
#'user/p
user=> (.f ^I p)

ClassCastException user.proxy$java.lang.Object$I$383c225e cannot be cast to user.I  user$eval7491.invokeStatic (:1)

After patch:

user=> (definterface I (f []))
user.I
user=> (def p (proxy [Object I] [] (f [] 1)))
#'user/p
user=> (definterface I (f []))
user.I
user=> (def p (proxy [Object I] [] (f [] 1)))
#'user/p
user=> (.f ^I p)
1

Cause: `proxy` caches the generated class using a set of classnames (see https://github.com/clojure/clojure/blob/master/src/clj/clojure/core_proxy.clj#L280-L286), this is not safe under redefinition of interfaces

Proposed: change the hashing function used to determine proxy class cache hits to take into account the identity of each interface/super class rather than just their name

Patch: 0001-CLJ-2379-idempotent-proxy-name-just-on-identical-ins-v2.patch



 Comments   
Comment by Alex Miller [ 02/Aug/18 4:50 AM ]

Can you add a Proposed line to the description explaining the change?

Comment by Alex Miller [ 02/Aug/18 4:52 AM ]

I'm not sure I get the problem/solution here.

Comment by Nicola Mometto [ 02/Aug/18 4:52 AM ]

sure, done

Comment by Alex Miller [ 02/Aug/18 5:06 AM ]

Looks like this breaks the serialized objects in the (admittedly fragile) clojure.test-clojure.java-interop/test-proxy-non-serializable (see CLJ-2204, CLJ-2330).

Comment by Nicola Mometto [ 02/Aug/18 10:26 AM ]

As discussed in #clojure-dev, attached patch that disables the brittle tests and restores the determinism of `proxy-name`.





[CLJ-1730] Improve `refer` performance Created: 13/May/15  Updated: 01/Aug/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: ft, performance

Attachments: Text File clj-1730-2.patch     Text File refer-perf.patch     PNG File Screenshot from 2018-08-01 23-23-41.png    
Patch: Code
Approval: Vetted

 Description   

refer underlies require, use, and refer-clojure use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

Approach: Some improvements that can be made:

  • Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
  • Use transients to build map of references to refer
  • Instead of cas'ing each new reference individually, build map of all changes, then cas
  • For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements in practice are modest.

Performance:

expr in a new repl 1.7.0-beta3 1.7.0-beta3+patch
(in-ns 'foo) (clojure.core/refer 'clojure.core) 2.65 ms 0.994 ms
(in-ns 'bar) (clojure.core/refer 'clojure.core :only '[inc dec]) 1.04 ms 0.113 ms
(use 'criterium.core) 0.877 ms 0.762 ms
(require '[clojure.core.async :refer (>!! <!! chan close!)]) 3408 ms 3302 ms

Patch: clj-1730-2.patch

Screening Notes

Patch appears correct but we should consider:

  • non-idiomatic use of if instead of when makes branches hard to read
  • non-idiomatic indentation of if (both branches on one line) hard to read
  • I don't think not found should be an IllegalAccessError, but the original code did this already, so ...
  • the optimistic concurrency loop around the swap will never give up, is this ok?


 Comments   
Comment by Alex Miller [ 07/Sep/17 10:52 AM ]

Patch updated to address first screening comment. I didn't actually find any case of the second one? Give me a pointer.

Comment by Ghadi Shayban [ 01/Aug/18 10:30 PM ]

I suspect that this will not matter much for peak performance. Given the attached picture of clojure.instant loading, this patch may shave a bit off startup time.

(picture made with Bytestacks, profile recorded a JDK11 debug build from today)





[CLJ-2348] [spec] 'check' has inconsistent behavior for required and optional map keys Created: 16/Apr/18  Updated: 01/Aug/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-2003] [spec] Nesting cat inside ? causes unform to return nested result Created: 11/Aug/16  Updated: 31/Jul/18

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

Type: Defect Priority: Critical
Reporter: Sam Estep Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: spec

Attachments: Text File CLJ-2003-corrected.patch     Text File CLJ-2003.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Calling conform and then unform with a spec that consists of some cat nested inside of some ? creates an extra level of nesting in the result:

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

(let [spec (s/? (s/cat :foo #{:foo}))
      initial [:foo]
      conformed (s/conform spec initial)
      unformed (s/unform spec conformed)]
  [initial conformed unformed])
;;=> [[:foo] {:foo :foo} [(:foo)]]

This behavior does not occur with just ? or cat alone:

(let [spec (s/? #{:foo})]
  (s/unform spec (s/conform spec [:foo])))
;;=> [:foo]

(let [spec (s/cat :foo #{:foo})]
  (s/unform spec (s/conform spec [:foo])))
;;=> (:foo)

Patch: CLJ-2003-corrected.patch



 Comments   
Comment by Phil Brown [ 14/Aug/16 9:55 PM ]

I came across another case of extra nesting, when repeating one or more sequences with an optional element at the beginning or end, where that element's predicate also matches the element at the other end:

user=> (s/conform (s/+ (s/cat :k any? :v (s/? any?))) [:a 1 :b 2])
[{:k :a, :v 1} [{:k :b, :v 2}]]

where I expected

[{:k :a, :v 1} {:k :b, :v 2}]

The following give expected results:

user=> (s/conform (s/+ (s/cat :k any? :v (s/? any?))) [:a 1 :b])
[{:k :a, :v 1} {:k :b}]
user=> (s/conform (s/+ (s/cat :k keyword? :v (s/? int?))) [:a 1 :b 2])
[{:k :a, :v 1} {:k :b, :v 2}]
user=> (s/conform (s/* (s/cat :k any? :v (s/? any?))) [:a 1 :b 2])
[{:k :a, :v 1} {:k :b, :v 2}]
Comment by Alex Miller [ 01/Sep/16 11:06 AM ]

Phil, I think your example is a different issue and you should file a new jira for that.

Comment by Alex Miller [ 01/Sep/16 3:05 PM ]

Well, maybe I take that back, they may be related.

Comment by Brandon Bloom [ 08/Nov/16 6:10 PM ]

I just ran in to this trying to make sense of some defn forms. Here's an example:

user=> (s/unform :clojure.core.specs/defn-args (s/conform :clojure.core.specs/defn-args '(f [& xs])))
(f ((& xs)))

Comment by Tyler Tallman [ 09/Aug/17 9:41 PM ]

This seems to be all that is needed.

Comment by Francis Avila [ 28/Aug/17 9:24 PM ]

The problem is actually more universal than (? (cat ...)). s/unform of a s/? with any regex child op will introduce an extra level of nesting. When the child is a regex, we are consuming the same "level" of sequence so unform should not introduce an extra level. However in other cases (non-regex ops), we should still possibly produce a nested collection.

The previous patch was too aggressive: it unwrapped all sub-unforms of s/?. This patch CLJ-2003-corrected.patch only unwraps when the sub-op is a regex.

Unfortunately it is impossible to distinguish between a desired-but-optional nil and a non-match from s/?. Specifically, the following tests now hold:

(testing "s/? matching nil"
  (is (nil? (s/conform (s/? nil?) [nil])))
  (is (nil? (s/conform (s/? nil?) [])))
  (is (nil? (s/conform (s/? nil?) nil)))
  (is (= (s/unform (s/? nil?) nil) [])))

(I did not add these tests to the patch because I was unsure if they should be part of the contract of unform. However, they are pretty big gotchas.)

I also added tests for every possible subop of s/?, except ::s/accept, which I could not think of a test case for. (I'm not sure ::s/accept is actually reachable inside s/op-unform?)

Comment by Alex Miller [ 29/Aug/17 7:33 AM ]

Thanks for working on this - I will take a look when I get a chance.

Comment by Francis Avila [ 29/Aug/17 9:20 AM ]

I had to amend my patch slightly (same name): one of the test cases wasn't testing the correct thing.

Comment by Francis Avila [ 31/Jul/18 1:12 PM ]

Patch was no longer applying cleanly to master because of tests added by other commits. Patch rebased to master





[CLJ-1954] clojure.set/intersection mishandles vectors Created: 09/Jun/16  Updated: 30/Jul/18

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

Type: Defect Priority: Major
Reporter: Ashton Kemerling Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: set


 Description   

clojure.set/intersection appears to use the indexes of vectors as values. This results in very strange behavior if you accidentally end up passing a vector in as one of the arguments.

ti.repl-init=> (clojure.set/intersection #{0 1} [2 2 2 2 2])
#{0 1}
ti.repl-init=> (clojure.set/intersection [2 2 2 2] #{0 1})
#{0 1}
ti.repl-init=> (clojure.set/intersection [0 1] [2 2 2 2])
[0 1]
ti.repl-init=> (clojure.set/intersection [2 2 2 2] [2 2 2 2])
[2 2 2 2]
ti.repl-init=> (clojure.set/intersection [3 3 3 ] [2 2 2 2])
[3 3 3]
ti.repl-init=> (clojure.set/intersection [55] [2 2 2 2])

ClassCastException clojure.lang.PersistentVector cannot be cast to clojure.lang.IPersistentSet  clojure.core/disj (core.clj:1476)

If any of the arguments are lists, you get a ClassCastException which is maybe a bit less clear than one would hope.

ti.repl-init=> (clojure.set/intersection #{0 1} (list 2 2 2 2))

IllegalArgumentException contains? not supported on type: clojure.lang.PersistentList  clojure.lang.RT.contains (RT.java:814)

The same also happens if all arguments are lists:



 Comments   
Comment by Ashton Kemerling [ 09/Jun/16 9:44 AM ]

More odd side effects.

ti.repl-init=> (clojure.set/intersection #{:foo} {:foo 1})
#{:foo}
ti.repl-init=> (clojure.set/intersection #{:foo} {})
{}
ti.repl-init=> (clojure.set/intersection #{:foo} [:foo])
#{}
ti.repl-init=> (clojure.set/intersection [:foo] [:foo])

ClassCastException clojure.lang.PersistentVector cannot be cast to clojure.lang.IPersistentSet  clojure.core/disj (core.clj:1476)
ti.repl-init=> (clojure.set/intersection [0] [:foo])
[0]
Comment by Alex Miller [ 09/Jun/16 9:54 AM ]

See comments on CLJ-1953

Comment by Andy Fingerhut [ 30/Jul/18 7:09 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





[CLJ-1682] clojure.set/intersection occasionally allows non-set arguments. Created: 24/Mar/15  Updated: 30/Jul/18

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

Type: Defect Priority: Minor
Reporter: Valerie Houseman Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: checkargs

Approval: Triaged

 Description   

clojure.set/intersection, by intent and documentation, is meant to be operations between two sets. However, it sometimes allows (and returns correct opreations upon) non-set arguments. This confuses the intention that non-set arguments are not to be used.

Here's an example with Set vs. KeySeq:
If there happens to be an intersection, you'll get a result. This may lead someone coding this to think that's okay, or to not notice they've used an incompatible data type. As soon as the intersection is empty, however, an appropriate type error ensues, albeit by accident because the first argument to clojure.core/disj should be a set.

user=> (require '[clojure.set :refer [intersection]])
nil
user=> (intersection #{:key_1 :key_2} (keys {:key_1 "na"}))   ;This works, but shouldn't
(:key_1)
user=> (intersection #{:key_1 :key_2} (keys {:key_3 "na"}))   ;This fails, because intersection assumes the second argument was a Set
ClassCastException clojure.lang.APersistentMap$KeySeq cannot be cast to clojure.lang.IPersistentSet  clojure.core/disj (core.clj:1449)

(disj (keys {:key_1 "na"}) #{:key_1 :key_2})   ;The assumption that intersection made
ClassCastException clojure.lang.APersistentMap$KeySeq cannot be cast to clojure.lang.IPersistentSet  clojure.core/disj (core.clj:1449)

Enforcing type security on a library that's clearly meant for a particular type seems like the responsible thing to do. It prevents buggy code from being unknowingly accepted as correct, until the right data comes along to step on the bear trap.



 Comments   
Comment by Andy Fingerhut [ 24/Mar/15 7:19 PM ]

CLJ-810 was similar, except it was for function clojure.set/difference. That one was declined with the comment "set/difference's behavior is not documented if you don't pass in a set." I do not know what core team will judge ought to be done with this ticket, but wanted to provide some history.

Dynalint [1] and I think perhaps Dire [2] can be used to add dynamic argument checking to core functions.

[1] https://github.com/frenchy64/dynalint
[2] https://github.com/MichaelDrogalis/dire

Comment by Alex Miller [ 24/Mar/15 9:00 PM ]

Now that `set` is faster for sets, I think we could actually add checking for sets in some places where we might not have before. So, it's worth looking at with fresh eyes.

Comment by Jason Wolfe [ 28/May/15 2:54 AM ]

Back in 2009 I submitted a patch to the set functions with explicit `set?` checks and Rich's response was "the fact that these functions happen to work when the second argument is not a set is an implementation artifact and not a promise of the interface, so I'm not in favor of the set? testing or any other accommodation of that." Not sure if that is still accurate though.

Comment by Andy Fingerhut [ 30/Jul/18 7:08 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





[CLJ-1953] clojure.set should check or throw on non-set inputs Created: 09/Jun/16  Updated: 30/Jul/18

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

Type: Defect Priority: Major
Reporter: Ashton Kemerling Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: set
Environment:

Not Relevant



 Description   

clojure.set/union is very sensitive to the types of its inputs. It does not attempt to check or fix the input types, raise an error, or even document this behavior.

If all inputs are sets, it works.

ti.repl-init=> (clojure.set/union #{1 2 3} #{1 2 3 4})
#{1 4 3 2}

If the arguments are both vectors or sequences, it returns the same type with duplicates.

ti.repl-init=> (clojure.set/union [1 2 3] [1 2 3])
[1 2 3 1 2 3]
ti.repl-init=> (clojure.set/union (list 1 2 3) (list 1 2 3))
(3 2 1 1 2 3)

If the arguments are mixed, the correct result is returned only if the longest input argument is a set.

ti.repl-init=> (clojure.set/union #{1 2 3} [2 3])
#{1 3 2}
ti.repl-init=> (clojure.set/union [1 2 3] #{2 3})
[1 2 3 3 2]
ti.repl-init=> (clojure.set/union [2 3] #{1 2 3})
#{1 3 2}
ti.repl-init=> (clojure.set/union #{2 3} [1 2 3])
[1 2 3 3 2]


 Comments   
Comment by Alex Miller [ 09/Jun/16 9:40 AM ]

This has been raised a number of times. See CLJ-1682, CLJ-810.

Comment by Ashton Kemerling [ 09/Jun/16 9:52 AM ]

I do not see set/union being covered in the tickets you mentioned.

Furthermore, this issue differs from the intersection bugs in a few ways important ways:

  1. It silently returns data that is the wrong type, and which contains the wrong values.
  2. It never raises an exception.

But it does share the following bugs with the intersection problem:

  1. This behavior is not only type dependent, but data dependent. It will happen to work depending on the lengths of the given sets.
  2. It isn't even documented that this function expects sets.
  3. It runs directly contrary to the definition of the mathematical function it purports to represent.

I only caught this bug in my own code because I hand inspected the result. I had just assumed that set/union would do the right thing, and was deeply surprised when against both definition and documentation it did not.

Comment by Andy Fingerhut [ 09/Jun/16 11:07 AM ]

I am sympathetic to your desires, Ashton, but have no new arguments that might convince those who decide what changes are made to Clojure that it would be a good enough idea to do so.

I would point out an answer to one of your comments: "It isn't even documented that this function expects sets." It seems to me from past comments that the point of view of the Clojure core team is that this is documented, e.g. "Return a set that is the union of the input sets" tells you what clojure.set/union does when you give it sets as arguments. It specifies nothing about what it does when you give it non-set arguments, so it is free to do anything at all in those cases, including what it currently does.

Comment by Andy Fingerhut [ 30/Jul/18 7:07 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





[CLJ-2021] [core.specs] defn spec does not unform arg list vectors Created: 12/Sep/16  Updated: 28/Jul/18

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

Type: Defect Priority: Major
Reporter: Jeroen van Dijk Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: core.specs, spec
Environment:

clojure 1.9, mac osx, java 1.8


Approval: Triaged

 Description   

The example belows shows a case where a conform-ed form, does not conform any after an unform. It would be my expectation that you can repeat conform -> unform -> conform endlessly and get the same result.

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

(s/def ::defn-macro (s/cat :type #{'defn} :definition :clojure.core.specs/defn-args))

(let [form '(defn foo "bar" ([a & b] a a c) ([a b] a))]

  (-> form
      (->> (s/conform ::defn-macro))) ;;=> {:type defn, :definition {:name foo, :docstring "bar", :bs [:arity-n {:bodies [{:args {:args [[:sym a]], :varargs {:amp &, :form [:sym b]}}, :body [a a c]} {:args {:args [[:sym a] [:sym b]]}, :body [a]}]}]}}

  ;; Unforming returns the function definition, but with the args in a list instead of a vector:
  (->> form
       (s/conform ::defn-macro)
       (s/unform ::defn-macro))  ;;=> (defn foo "bar" ((a (& b)) a a c) ((a b) a))) 

  ;; Conforming after unforming doesn't work anymore
  (->> form
       (s/conform ::defn-macro)
       (s/unform ::defn-macro)
       (s/conform ::defn-macro)) ;;=> :clojure.spec/invalid

    )


 Comments   
Comment by Jeroen van Dijk [ 12/Sep/16 8:22 AM ]

This gist shows the above code with better formatting https://gist.github.com/jeroenvandijk/28c6cdd867dbc9889565dca92673a531

Comment by Leon Grapenthin [ 28/Jan/17 4:49 PM ]

This can quickly be traced down to :clojure.core.specs/arg-list which is speced as a (s/and <regex> vector?). When unforming, it doesn't create a vector.

Thinking about it, a vcat would be nice for this and similar cases.

Comment by Marco Molteni [ 28/Jul/18 9:27 AM ]

Hello, any news ?

Comment by Alex Miller [ 28/Jul/18 3:14 PM ]

Nope.





[CLJ-1487] Variadic unrolling for partial Created: 01/Aug/14  Updated: 26/Jul/18  Resolved: 26/Jul/18

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

Type: Enhancement Priority: Minor
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None

Attachments: Text File unroll-partial.patch    
Patch: Code and Test

 Description   

Many of the functions in clojure.core are variadic-unrolled for performance. The implementation of juxt, for example, is 29 lines where it could be just 3 if we didn't care about performance. For the function "partial", this unrolling is, if you will excuse the pun, only partially done. This patch extends the unrolling for partial to mirror that for juxt and comp, and includes a test that partial works for any reasonable number of arguments (the existing test just spot-checks a few arities).

I've done some performance benchmarking, and we get about a 20% speedup on calling partial functions, with no performance penalty for creating them.

You may note that the n-ary case, ([arg1 arg2 arg3 & more] ...), is not unrolled in my patch. Doing so would have worsened performance: because we're having to call apply anyway, there's no real benefit to be had from unrolling, and it costs a little to rebuild the arglist before passing it to apply.



 Comments   
Comment by Ghadi Shayban [ 01/Aug/14 6:45 PM ]

Dupe of CLJ-1430

Comment by Alex Miller [ 01/Aug/14 11:10 PM ]

Dupe as noted in the comments





[CLJ-2113] Update Clojure maven for latest on CI server Created: 16/Feb/17  Updated: 26/Jul/18  Resolved: 26/Jul/18

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: build

Attachments: Text File mvn3.patch    
Patch: Code
Approval: Ok

 Description   

Update maven build infrastructure to stop using oss-parent and use updated Maven 3 and sonatype plugins (similar to other changes made recently in all contrib projects).

  • Removed oss-parent parent pom. This has been deprecated for years and is no longer recommended for use.
  • Add snapshot repo (was previously pulled in via oss-parent)
  • maven-compiler-plugin - update to latest version
  • maven-release-plugin - update to latest version
  • add nexus-staging-maven-plugin - current recommended plugin for releases to maven central, replaces most of the maven-release-plugin's work (old version of this previously in oss-parent)
  • add maven-gpg-plugin for signing (previously in oss-parent)
  • remove old release profile which was activated by oss-parent pom

Patch: mvn3.patch

It's difficult to test this completely outside the context of actually building and deploying snapshots and releases but the changes are very similar to those made for all contrib projects recently.






[CLJ-1852] Clojure-generated class names length exceed file-system limit Created: 20/Nov/15  Updated: 26/Jul/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: Martin Raison Assignee: Unassigned
Resolution: Unresolved Votes: 11
Labels: compiler
Environment:

tested on CentOS 6


Attachments: Text File 0001-CLJ-1852-Create-jar-if-class-name-length-exceed-MAX_.patch    
Approval: Triaged

 Description   

Class names generated by the Clojure compiler can be arbitrarily long, exceeding the file system's maximum allowed file name length. For example it happens when you nest functions a bit too deeply:

(defmacro nestfn [n & body]
  (if (> n 0)
    `(fn [] (nestfn ~(- n 1) ~@body))
    body))

(def myf (nestfn 100 "body"))

Compiling this produces a java.io.IOException: File name too long exception.



 Comments   
Comment by Martin Raison [ 20/Nov/15 9:32 PM ]

The Scala community found this issue a while ago, and now the compiler has a max-classfile-name parameter (defaulting to 255). Hashing is used when the limit is exceeded. Maybe we should consider something similar?

Comment by Philipp Neumann [ 16/Oct/17 10:53 AM ]

I tried clojure.core.match with 13 patterns and the compiliation failed under Windows. I assume this problem is the root cause of it.

Comment by Dr. Christian Betz [ 08/Mar/18 4:47 AM ]

Some more info on that:

A colleague of mine just ran into that problem because he's using Linux / eCryptfs (where the limit of 143 is rather small, compared to our FileVault encrypted macOS used otherwise): see https://bugs.launchpad.net/ecryptfs/+bug/344878.

However, Clojure's not alone with that problem, Scala is also hit hard: https://issues.scala-lang.org/browse/SI-3623

There's no "easy" solution to this, and truncating the filename (as Scala does) bears a lot of other problems, obviously.

For all of you bitten by this problem, one possible workaround might be the one proposed by Mario Pastorelli (https://issues.scala-lang.org/secure/ViewProfile.jspa?name=melrief) in comment https://issues.scala-lang.org/browse/SI-3623?focusedCommentId=76104&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-76104:

Use an unencrypted filesystem to temporarily store classfiles, maybe by having a tempFS to keep stuff in memory. Not the best way, because we do not like leaving important stuff unencrypted, ...

Comment by Alex Vong [ 16/Mar/18 4:55 PM ]

Hello,

I run into the same problem while using clojure.core.match. I macroexpand the function definition and observe that the macro-expanded definition is deeply nested.

I think one way to solve it is to provide an option to wrap the class file as a jar with some shorter file name so that the the class name can remain the same (zipped file name can be as long as you like, right?). WDYT?

Btw, I am using clojure 1.9.0 so I think we should say that this bug affects 1.9 as well.

Comment by Alex Vong [ 17/Mar/18 11:50 AM ]

I come up with a proof-of-concept patch. The compiler now outputs jar instead of class if the class name is longer than 255. The jar name is simply a (left) truncation of the class name.

Suppose *compile-path* is set to "build", then you need to add build/* to your class path, so that the jars can be found.

This patch is only a proof of concept, ideally all classes with long name should be put into one big jar to avoid having to decompress many files. Also, the user should be able to specify *compile-name-max* and *compile-jar-name*. Finally, the code is quite ugly, I should have spitted things into several functions.

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

Alex - we're not going to output jars. This is at odds with many facets of the Clojure runtime.

Comment by Steve Miner [ 17/Mar/18 4:49 PM ]

Sorry if this comment is off topic, but the original example is a bit confusing to me. Maybe it should be:

(defmacro nestfn [n & body]
  (if (> n 0)
    `(fn [] (nestfn ~(dec n) ~@body))
    `(do ~@body)))

That way (trampoline (nestfn 10 "foo")) would return "foo". However, I do get a CompilerException java.lang.StackOverflowError for n=1000 on macOS.

Comment by Ivan Kryvoruchko [ 26/Jul/18 4:57 AM ]

@Alex Miller Hello! Would a patch with Scala-like approach (hashing of name in case it's too long) be considered? As it breaks binary compatibility this workaround would definitely be disabled by default, but it would be possible to enable name hashing by using new compiler option. This approach would definitely help in our particular case, but I'm not sure if it's useful/generic enough to be included in compiler, so I haven't started working on patch yet.

Comment by Alex Miller [ 26/Jul/18 9:25 AM ]

Not going to make any breaking changes. Plan should be to switch second strategy when current strategy doesn’t work.





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

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: 1
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






[CLJ-2037] [spec] specs in registry lack :file metadata despite having :line, :column Created: 08/Oct/16  Updated: 24/Jul/18

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

Type: Enhancement Priority: Major
Reporter: Felix Andrews Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: spec

Approval: Triaged

 Description   

As of 1.9.0-alpha13, specs in the registry lack :file metadata despite having :line, :column

user=> (require '[clojure.spec :as s])
user=> (-> (s/registry) (get :clojure.core.specs/arg-list) (meta))
{:line 1118, :column 5, :clojure.spec/name :clojure.core.specs/arg-list}
user=> (-> (s/registry) (get 'clojure.core/let) (meta))
{:line 1675, :column 5, :clojure.spec/name clojure.core/let}

This would be useful because:

  • we could list all the specs defined in a project, by filtering the registry.
  • we could read the source of a spec, like clojure.repl/source, for pretty formatting.

(specifically, for use in Codox https://github.com/weavejester/codox/pull/134 )

I had a quick look but couldn't see where the metadata is set.
Cheers



 Comments   
Comment by Alex Miller [ 08/Oct/16 11:12 AM ]

You can use s/describe or s/form to grab the source of a spec now, btw.

Comment by Felix Andrews [ 12/Oct/16 11:29 PM ]

The following works in my tests. (For testing I used in-ns, @#'registry-ref, #'ns-qualify)).

The approach is to set the registry item metadata after a def. It is not enough to set metadata on the def'd value because it is subsequently altered inside def.

(ns clojure.spec)
(alias 'c 'clojure.core)

(defmacro def
  [k spec-form]
  (let [k (if (symbol? k) (ns-qualify k) k)
        m (assoc (meta &form) :file *file*)]
    `(do
       (def-impl '~k '~(res spec-form) ~spec-form)
       (swap! registry-ref update '~k vary-meta c/merge ~m)
       '~k)))

(defmacro fdef
  [fn-sym & specs]
  (let [k (ns-qualify fn-sym)
        m (assoc (meta &form) :file *file*)]
    `(do
       (clojure.spec/def ~fn-sym (clojure.spec/fspec ~@specs))
       (swap! registry-ref update '~k vary-meta c/merge ~m)
       '~k)))

You can use s/describe or s/form to grab the source of a spec now, btw.

Yes, that's nice except for longer specs when line wrapping and indentation would help.

Comment by Jozef Wagner [ 01/Dec/16 12:31 PM ]

Note that current :line and :column meta are not pointing to the place where the spec was defined but to the clojure/spec.clj file, e.g. second example (c.c/let) points to fspec-impl

Comment by Martin Klepsch [ 24/Jul/18 11:40 AM ]

I've been looking into fixing this and for specs that aren't ident? we can use metadata or the map describing regex? specs.

For ident? specs I'm not sure how to handle the additional data — it could be stored in some other location of the registry which might be a viable path.

With any of these approaches it's unclear to me how to integrate this information into the return value of get-spec in a consistent way since it's return value may not be maps (therefore not extensible.)

Storing them in a different location in the registry would work as mentioned before but then we would need another function or protocol method to get information about source file/line/column.





[CLJ-2350] Improve error message when calling a keyword with the wrong number of arguments Created: 03/May/18  Updated: 20/Jul/18

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

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

Attachments: Text File keyword-arity-exception-01.patch     Text File keyword-arity-exception-02.patch     Text File keyword-arity-exception-03.patch     Text File keyword-arity-exception.patch    
Patch: Code and Test
Approval: Incomplete

 Description   

When you call a Keyword with the wrong number of arguments, the error message does not report how many arguments were passed:

(:kw "one" "two" "three")
=> java.lang.IllegalArgumentException: Wrong number of args passed to keyword: :kw

compare to calling an IFn, which does show the number of arguments passed:

(name "one" "two" "three")
=> clojure.lang.ArityException: Wrong number of args (3) passed to: core/name

The latter error message is more clear and makes it easier to debug.

The attached patch re-uses the ArityException class used elsewhere to generate error messages in the latter form when calling a keyword with the wrong number of arguments.

Addresses CLJ-1067

Patch: keyword-arity-exception-03.patch

Prescreened by: Alex Miller



 Comments   
Comment by Marc O'Morain [ 03/May/18 7:51 AM ]

Fix typo: later => latter.

Comment by Alexander Taggart [ 03/May/18 10:37 AM ]

+100 for improving error messages.

Albeit unlikely, since `throwArity()` is a public method, removing it risks breaking external code.

Less importantly, the resulting sentence looks a bit awkward with the three colons:
"Wrong number of args (3) passed to: keyword: :my.ns/foo"

Alternatively:
"Wrong number of args (3) passed to: keyword :my.ns/foo"
"Wrong number of args (3) passed to: :my.ns/foo"

The latter parallels the result when calling a non-keyword function:
"Wrong number of args (3) passed to: my.ns/bar"

Comment by Alex Miller [ 03/May/18 11:39 AM ]

Also, actually there is a typo artity-exceptions in the test.

And I would agree with Alexander's comment, should leave existing throwArity().

Comment by Marc O'Morain [ 04/May/18 6:34 AM ]

Attach updated patch.

Comment by Marc O'Morain [ 04/May/18 6:40 AM ]

I've attached an updated patch that fixes the type in the deftest declaration and formats the exception message in the way Alexander suggested.

Alex - should I add a Java comment to explain why the 0-arity version of Keyword.throwArtity() exists? Something like:

/**
 * @deprecated CLJ-2350 This function is no longer called, but has not been removed to maintain the public interface.
 */

Reflecting on the public interface, perhaps we could make the new function, throwArity(int n), package-private. This would avoid it becoming part of the public API of clojure.lang.

Comment by Alex Miller [ 04/May/18 11:56 PM ]

yes and yes

Comment by Marc O'Morain [ 07/May/18 3:26 PM ]

Added patch making throwArity(int) package private, and adding deprecation docstring.

Comment by Alex Miller [ 29/Jun/18 11:10 AM ]

The final invoke takes a variadic as the final arg so it's actually 21+, not 21. Could count the args array. The patch has also drifted from master and needs to be rebased.

Comment by Marc O'Morain [ 09/Jul/18 5:40 PM ]

Added patch that improves the message for the 21+ arg case.

Comment by Marc O'Morain [ 09/Jul/18 5:42 PM ]

There are two more places in the code that have the same bug in the 21+ arg case:

https://github.com/clojure/clojure/blob/71511b7800e18c83377a322f43585a853b303698/src/jvm/clojure/lang/RestFn.java#L4078
https://github.com/clojure/clojure/blob/71511b7800e18c83377a322f43585a853b303698/src/jvm/clojure/lang/AFn.java#L140

Would you like an additional patch to cover those cases?

Comment by Alex Miller [ 09/Jul/18 6:56 PM ]

Sure, separate ticket though.

Comment by Marc O'Morain [ 20/Jul/18 7:02 AM ]

Hi Alex, any update on this ticket? I've updated the patch to the latest master as requested, so it should be good to merge now.

Comment by Alex Miller [ 20/Jul/18 5:08 PM ]

I’ll get to it next time I cycle through.





[CLJ-1975] [spec] clojure.spec attempts to make `empty` records Created: 05/Jul/16  Updated: 19/Jul/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: 3
Labels: spec
Environment:

1.9.0-alpha11


Approval: Triaged

 Description   
user> (require '[clojure.spec :as s])
nil
user> (defrecord Box [a])
user.Box
user> 
user> (s/conform
        (s/cat :boxes (s/* #(instance? Box %))
               :name (s/coll-of integer?))
        [(Box. 0) [5]])
UnsupportedOperationException Can't create empty: user.Box  user.Box (form-init8049111656025227309.clj:1)
user> (clojure.repl/pst *e)
UnsupportedOperationException Can't create empty: user.Box
       	user.Box (NO_SOURCE_FILE:2)
	clojure.core/empty (core.clj:5151)
	clojure.spec/every-impl/cfns--14008/fn--14014 (spec.clj:1215)
	clojure.spec/every-impl/reify--14027 (spec.clj:1229)
	clojure.spec/conform (spec.clj:150)
	clojure.spec/dt (spec.clj:731)
	clojure.spec/dt (spec.clj:727)
	clojure.spec/deriv (spec.clj:1456)
	clojure.spec/deriv (spec.clj:1463)
	clojure.spec/deriv (spec.clj:1467)
	clojure.spec/re-conform (spec.clj:1589)
	clojure.spec/regex-spec-impl/reify--14267 (spec.clj:1633)

This is a regression from -alpha7; the same sort of spec (modulo the default-value arg to `coll-of`) works as expected there.



 Comments   
Comment by Alex Miller [ 02/Nov/17 3:13 PM ]

In this case, it's considering the s/* to be a non-match and then matching the (Box. 0) against (s/coll-of integer?). This matches the initial predicate (coll?) but falls through to the :else case which presumes it can call `empty`.

You can work around it by adding a :kind predicate to the coll-of:

(s/conform
        (s/cat :boxes (s/* #(instance? Box %))
               :name (s/coll-of integer? :kind #(not (record? %))))
        [(Box. 0) [5]])
;;=> {:boxes [#user.Box{:a 0}], :name [5]}
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?





[CLJ-2168] [spec] clojure.spec: :pred in explain for coll-of should have resolved symbols Created: 26/May/17  Updated: 18/Jul/18

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

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

0.1.123


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

 Description   

:pred should be resolved in explain problems like:

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

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

should be

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

Other examples:

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

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

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

Patch: clj-2168.patch



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

The same problem happens with s/every.

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

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

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

For example,

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

I'll file that as another ticket later.

Comment by Alexander Kiel [ 18/Jul/18 7:56 AM ]

Issue https://github.com/alexanderkiel/phrase/issues/22 relates to this.





[CLJ-1752] realized? return true for an instance that is not IPending Created: 09/Jun/15  Updated: 17/Jul/18

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

Type: Enhancement Priority: Minor
Reporter: Logan Linn Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File 0001-CLJ-1752-realized-return-true-for-an-instance-that-i.patch    
Approval: Triaged

 Description   

To safely test if an arbitrary seq is realized (non-lazy), we need a wrapper like:

(defn seq-realized? [s]
  (if (instance? clojure.lang.IPending s)
    (realized? s)
    true))

If realized? returned true for an (ISeq?) instance that is not IPending there would be less surprising behavior for cases such as, (realized? (range 10)) which throws exception.

NB: A follow-up to CLJ-1751.



 Comments   
Comment by Marc O'Morain [ 17/Jul/18 3:54 PM ]

Attaching a patch that makes realized? return true for non-pending xs.

Comment by Marc O'Morain [ 17/Jul/18 3:56 PM ]

Scratch that - patch is no good.

Comment by Marc O'Morain [ 17/Jul/18 6:26 PM ]

Attaching a patch that makes realized? return true for non-pending xs. Add tests for the IPending types.





[CLJ-2378] [spec] instrument should check :ret and :fn specs Created: 16/Jul/18  Updated: 17/Jul/18  Resolved: 16/Jul/18

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

Type: Feature Priority: Major
Reporter: jcr Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: spec


 Description   

Problem: a function instrumented via clojure.spec.test.alpha/instrument checks the :args part of its spec, but not the :ret and :fn parts. It's confusing and forces people to use a 3rd-party library[1].

Solution: add an option to turn on :ret and :fn checking to clojure.spec.test.alpha/instrument.

[1]: https://github.com/jeaye/orchestra



 Comments   
Comment by Alex Miller [ 16/Jul/18 5:59 PM ]

The basic idea here is that instrument is designed to verify that a function is invoked correctly. For determining whether the internals of a function are correct, you can use `check`.

This has been discussed at great length, both on the core team and in the broader community and we’re familiar with all the arguments. While I’m not ruling out the possibility that we will do something in this area in the future, we are not currently planning on changing this, so I’m going to decline this for now. I’ll repeat, saying no to this now does not prevent us from saying yes later.

Comment by Ghadi Shayban [ 16/Jul/18 6:04 PM ]

https://dev.clojure.org/display/community/Creating+Tickets
https://groups.google.com/forum/#!topic/clojure/RLQBFJ0vGG4/discussion
There's also some commentary from Rich in Slack, but I don't have a link handy.
("we should do X" tickets are almost universally declined.)
dup of https://dev.clojure.org/jira/browse/CLJ-1961

Comment by jcr [ 17/Jul/18 10:44 AM ]

Sorry, I've only searched through open issues before creating this ticket.

Is it at all possible to keep this issue open so people could vote for it and subscribe to it, since (as Alex stated) it's not set in stone yet?


(The thing is, I was confused by this behavior and asked on irc and elsewhere, and was told by several people that it's weird and looks like a bug. I've filed it as a "feature" since the doc clearly states that's the intended behavior. Also, it's not quite clear how to use check to verify correctness of a function for a specific subset of possible inputs (think checking the fn in repl as you develop it) - maybe the spec guide could elaborate a bit more on what the supposed workflow should look like? Maybe that'd clear up some confusion; though, I understand that's out of the scope.)

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

As I said, we're aware of the request and why people want it. From a core team perspective, we don't need an issue to track it.

Use `check` if you want to use property based testing to verify the correctness of a function with respect to its spec. Generally if you want to test specific possible inputs, most people use example-based tests with clojure.test instead - that's not in the guide as it doesn't really have anything to do with spec. It is possible to override the args generator with a set spec of explicit inputs, but I'm not sure if the effort of that is worth the work most of the time.

Comment by jcr [ 17/Jul/18 12:57 PM ]

Well, fair enough!

Re example-based tests: what I had in mind is 1) calling a function (presumably in repl) with a specific input to see if the output still conforms to :ret and :fn specs; 2) testing a function A which calls spec'ed functions B1..Bn to see which of those fail their :ret/:fn specs (essentially it gives you free unit tests for each integration test). I agree that it's theoretically possible to do that in vanilla clojure.spec with a custom generator, but that certainly doesn't worth the effort.

Anyway, I guess you've already heard this argument, so I'll stop there.





[CLJ-2376] [core.specs] Check early if let binding vector is even Created: 13/Jul/18  Updated: 16/Jul/18

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

Type: Enhancement Priority: Minor
Reporter: Alexander Yakushev Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec

Attachments: Text File let-even.patch    
Patch: Code
Approval: Prescreened

 Description   

If you miss a value in the let binding vector, Spec will return a rather verbose and misguiding message:

user=> (require '[clojure.spec.alpha :as s] '[clojure.core.specs.alpha :as cs])
user=> (s/explain ::cs/bindings '[a #_1 b 2])
In: [2] val: 2 fails spec: :clojure.core.specs.alpha/local-name at: [:binding :sym] predicate: simple-symbol?
In: [2] val: 2 fails spec: :clojure.core.specs.alpha/seq-binding-form at: [:binding :seq] predicate: vector?
In: [2] val: 2 fails spec: :clojure.core.specs.alpha/map-bindings at: [:binding :map] predicate: coll?
In: [2] val: 2 fails spec: :clojure.core.specs.alpha/map-special-binding at: [:binding :map] predicate: map?

The crux of the problem here is the odd number of parameters in the binding. Spec gets to it anyway, but if it first finds a parameter that is an invalid binding form, it will bark at that instead. With the suggested patch, the error looks like:

user=> (s/explain ::cs/bindings '[a #_1 b 2])
val: [a b 2] fails spec: :clojure.core.specs.alpha/bindings predicate: (even? (count %))

Prescreened by: Alex Miller






[CLJ-2377] [spec] StackOverflowError in gen of recursive spec Created: 15/Jul/18  Updated: 15/Jul/18

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

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

Clojure 1.10.0-alpha6


Approval: Triaged

 Description   

Repro stripped down from larger piece of code:

clj -Srepro -Sdeps '{:deps {org.clojure/clojure {:mvn/version "1.10.0-alpha6"}, org.clojure/test.check {:mvn/version "0.10.0-alpha3"}}}'
(require '[clojure.spec.alpha :as s])

(s/def ::m (s/keys :req [::coll]))
(s/def ::coll (s/cat :m (s/? ::m)))

(s/conform ::m {::coll []})  ; => #:user{:coll {}}
(s/exercise ::m)  ; => StackOverflowError

Unlike in CLJ-2002, conform works fine here, but generation recurs for ever.






[CLJ-2259] Remove unnecessary bigdec? Created: 30/Oct/17  Updated: 15/Jul/18  Resolved: 31/Oct/17

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

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

Attachments: Text File clj-2259-2.patch     Text File clj-bigdec.patch     Text File spec-bigdec.patch    
Patch: Code
Approval: Ok

 Description   

In 1.9, we added bigdec? predicate, but the existing decimal? predicate is the same (this was unintentional).

Patches:

  • spec-bigdec.patch - spec.alpha patch to modify spec generator mapping
  • clj-2259-2.patch - Clojure patch to remove the new bigdec? predicate and update to latest spec (with updated mapping)


 Comments   
Comment by Marco Molteni [ 15/Jul/18 7:03 AM ]

Hello, please note that the Cognitect blog article http://blog.cognitect.com/blog/2016/8/9/focus-on-spec-predicates still mentions "bigdec?", which is confusing for people googling for the correct predicate.

Comment by Alex Miller [ 15/Jul/18 8:36 AM ]

I removed it from the blog post.





[CLJ-1047] Simplify the process of requiring fj in clojure.core.reducers Created: 21/Aug/12  Updated: 14/Jul/18  Resolved: 14/Jul/18

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: reducers

Attachments: Text File 001-simplify-fj-importing.patch    
Patch: Code

 Description   

this patch removes compile-if in favor of import-if, removing code duplication



 Comments   
Comment by David Bürgin [ 14/Jul/18 8:46 AM ]

compile-if is gone, this issue is now obsolete.





[CLJ-2375] Address JDK API deprecations Created: 11/Jul/18  Updated: 11/Jul/18

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

Type: Enhancement Priority: Minor
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop

Attachments: Text File CLJ-2375.patch    
Patch: Code
Approval: Vetted

 Description   

The JDK now has a clearer policy regarding API deprecations. Currently, there are 5 deprecated call sites in Clojure:

class clojure/pprint$translate_param uses deprecated method java/lang/Integer::<init>(Ljava/lang/String;)V 
class clojure/java/browse_ui$open_url_in_swing uses deprecated method java/awt/Window::show()V
class clojure/core$mix_collection_hash uses deprecated method java/lang/Long::<init>(J)V
class clojure/core$hash_ordered_coll uses deprecated method java/lang/Long::<init>(J)V
class clojure/core$hash_unordered_coll uses deprecated method java/lang/Long::<init>(J)V

The first two occur in clj files, and have clear replacement calls.

The last three (the Long(long) constructors) are emitted by the compiler while boxing invokeStatic and invokePrim. The Long constructors were deprecated in JDK9 [1], with the note that Hotspot intrinsifies the preferred call (Long/valueOf).

Addressing all of these remains compatible with JDK8.

This was found by building the local clojure uberjar and running jdeprscan --release <ver> clojure.jar
[1] https://docs.oracle.com/javase/10/docs/api/java/lang/Long.html#%3Cinit%3E(long)






[CLJ-2116] [spec] Support for selective conforming with clojure.spec Created: 22/Feb/17  Updated: 10/Jul/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: 35
Labels: spec
Environment:

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


Attachments: Text File clj-2116.patch    

 Description   

Problem

using clojure.spec in runtime border validation supporting multiple exchange formats is hard.

Details

Currently in clojure.spec (alpha-14), conformers are attached to Spec instances at creation time and they are invoked on every conform. This is not very useful in system border validation, where conforming/coercion functions should be selected based on runtime data, e.g. the exchange format.

Examples:

  • a keyword? spec:
    • with EDN, no coercion should be done (it can present Keywords)
    • with JSON, String->Keyword coercion should be applied
    • with String-based formats (CSV, query-params, ...), String->Keyword coercion should be applied
  • a integer? spec:
    • with EDN, no coercion should be done (it can present numbers)
    • with JSON, no coercion should be done (it can present numbers)
    • with String-based formats (CSV, query-params, ...), String->Long coercion should be applied

Here is a more complete example:

(s/def ::id integer?)
(s/def ::name string?)
(s/def ::title keyword?)
(s/def ::person (s/keys :opt [::id], :req-un [::name ::title]))

;; this is how we see the data over different exchange formats
(def edn-person {::id 1, :name "Tiina", :title :boss})
(def json-person {::id 1, :name "Tiina", :title "boss"})
(def string-person {::id "1", :name "Tiina", :title "boss"})

;; here's what we want
(def conformed-person edn-person)

To use this today, one needs to manually create new border specs with different conformers for all different exchange formats. Non-qualified keywords could be mapped in s/keys to work (e.g. ::title => ::title$JSON), but this wont work if fully qualified keys are exposed over the border (like ::id in the example) - one can't register multiple, differently conforming version of the spec with same name.

Suggestion

Support selective conforming in the Spec Protocol with a new 3-arity conform* and clojure.spec/conform, both taking a extra user-provided callback/visitor function. If the callback is provided, it's called from within the Specs conform* with the current spec as argument and it will return either nil or a 2-arity conformer function that should be used for the actual confrom.

Actual conforming-matcher implementations can be maintained in 3rd party libraries, like spec-tools[1].

Using it would look like this:

;; edn
(assert (= conformed-person (s/conform ::person edn-person)))
(assert (= conformed-person (s/conform ::person edn-person nil)))

;; json
(assert (= conformed-person (s/conform ::person json-person json-conforming-matcher)))

;; string
(assert (= conformed-person (s/conform ::person string-person string-conforming-matcher)))

Alternative

Another option to support this would be to allow Specs to be extended with Protocols. 3rd party libs could have a new Conforming protocol with 3-arity conform and add implementations for it on all current specs. Currently this is not possible.

[1] https://github.com/metosin/spec-tools



 Comments   
Comment by Alex Miller [ 22/Feb/17 3:33 PM ]

I don't think we are interested in turning spec into a transformation engine via conformers, so I suspect we are probably not interested. However, I'll leave it for Rich to assess.

Comment by Tommi Reiman [ 23/Feb/17 1:26 AM ]

Currently, Plumatic Schema is the tool used at the borders. Now, people are starting to move to Spec and it would really bad for the Clojure Web Developement Story if one had to use two different modelling libraries for their apps. If Spec doesn't want to be a tranformation engine via conformers, I hope for the Alternative suggestion to allow 3rd parties to write this kind of extensions: exposing Specs as Records/Types instead of reified protocols would do the job?

Comment by ken restivo [ 28/Feb/17 9:43 PM ]

I could see why the Clojure core developers might not want Spec to support this kind of coercion, but the practical reality is that someone will have to. If it isn't in Spec itself, it'll have to be done libraries built upon it like Tommi's.

The use case here is: I have a conf file that is YAML. I'm parsing the YAML using a Clojure library, turning it into a map. Now I have to validate the map, but YAML doesn't support keywords, for example, and the settings structure goes directly into Component/Mount/etc as part of the app state, so it makes sense to run s/conform on it as the first step in app startup after reading configuration. Add to this the possibility of other methods of merging in configuration (env vars, .properties files, etc) and this coercion will be necessary somewhere.

Comment by Tommi Reiman [ 08/May/17 1:03 PM ]

Any news on assessing this? I would be happy to provide a patch or a link to a modified clojure.spec with samples on usage with the 3-arity conform in it. Some thinking aloud: http://www.metosin.fi/blog/clojure-spec-as-a-runtime-transformation-engine/

Comment by Alex Miller [ 09/May/17 10:10 AM ]

Rich hasn't looked at it yet. My guess is still that we're not interested in this change. While I think some interesting problems are described in the post, I don't agree with most of the approaches being taken there.

Comment by Simon Belak [ 10/May/17 4:39 AM ]

Why not just use s/or (or s/alt) and then dispatch on the tag. Something like:

(s/def ::id (s/and (s/or :int integer?
                         :str string?)
                   (s/conformer (fn [[tag x]]
                                  (case tag
                                    :int x
                                    :str (Integer/parseInt x))))))

I use that pattern quite a bit in https://github.com/sbelak/huri and with a bit of syntactic sugar it works quite well.

Comment by Imre Kószó [ 12/May/17 3:46 AM ]

Simon that will not work if you are trying to conform to specs from third parties though. One of the points of this suggestion is that third parties would be able to write their own conformers to existing specs without redefining those specs.

Comment by Tommi Reiman [ 08/Jun/17 1:40 AM ]

Thanks for the comments. I would be happy to provide a patch / sample repo with the changed needed for this, in hope that it would help to decide if this could end up in the spec or not. What do you think?

Below is a sample of initial spec-integration into ring/http libs, using spec-tools. For now, one needs to wrap specs into spec records to enable the 3-arity conforming. This is boilerplate I would like to see removed. With this change, it should work out-of-box for all (3rd party) specs.

(require '[compojure.api.sweet :refer :all])
(require '[clojure.spec.alpha :as s])
(require '[spec-tools.core :as st])

;; to enable 3-arity conforming
(defn enum [values]
  (st/spec (s/and (st/spec keyword?) values)))

(s/def ::id int?)
(s/def ::name string?)
(s/def ::description string?)
(s/def ::size (enum #{:L :M :S}))
(s/def ::country (st/spec keyword?) ;; to enable 3-arity conforming
(s/def ::city string?)
(s/def ::origin (s/keys :req-un [::country ::city]))
(s/def ::new-pizza (st/spec (s/keys :req-un [::name ::size ::origin] :opt-un [::description])))
(s/def ::pizza (st/spec (s/keys :req [::id] :req-un [::name ::size ::origin] :opt-un [::description])))

;; emits a ring-handler with input & output validation (& swagger-docs)
;; select conforming based on request content-type (e.g. json/edn) + strip-extra keys from maps
(context "/spec" []
  (resource
    {:coercion :spec
     :parameters {:body-params ::new-pizza}
     :responses {200 {:schema ::pizza}}
     :post {:handler (fn [{new-pizza :body-params}]
                       (ok (assoc new-pizza ::id 1))}}))
Comment by Tommi Reiman [ 21/Jul/17 4:13 AM ]

Intended to create internal PR in my fork of clojure.spec, but ended up doing a real DUMMY PR for the actual repo. Well, here it is anyway:

https://github.com/clojure/spec.alpha/pull/1

Happy to finalize & create a patch into Jira if this goes any further.

Comment by Tommi Reiman [ 21/Jul/17 4:14 AM ]

comments welcome. here's a sample test for it:

(deftest conforming-callback-test
  (let [string->int-conforming
        (fn [spec]
          (condp = spec
            int? (fn [_ x _]
                   (cond
                     (int? x) x
                     (string? x) (try
                                   (Long/parseLong x)
                                   (catch Exception _
                                     ::s/invalid))
                     :else ::s/invalid))
            :else nil))]

    (testing "no conforming callback"
      (is (= 1 (s/conform int? 1)))
      (is (= ::s/invalid (s/conform int? "1"))))

    (testing "with conforming callback"
      (is (= 1 (s/conform int? 1 string->int-conforming)))
      (is (= 1 (s/conform int? "1" string->int-conforming))))))
Comment by Tommi Reiman [ 22/Jul/17 2:12 AM ]

initial work as patch.

Comment by Tommi Reiman [ 04/Oct/17 1:02 AM ]

Any news on this?

Comment by Tommi Reiman [ 31/Oct/17 12:05 PM ]

Related https://dev.clojure.org/jira/browse/CLJ-2251

Comment by Marco Molteni [ 10/Jul/18 5:11 PM ]

Hello, any news ?

Comment by Alex Miller [ 10/Jul/18 6:22 PM ]

We won’t look at this at least until we do the next batch of implementation changes. I continue to think we will most likely decline this.





[CLJ-2251] Generic spec walking for clojure.spec Created: 11/Oct/17  Updated: 10/Jul/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: 10
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-2374] Clojure master fails on JDK 11 EA builds due to overloaded toArray in gvec.clj Created: 10/Jul/18  Updated: 10/Jul/18

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

Type: Defect Priority: Major
Reporter: Karthikeyan Singaravelan Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: JDK11EA
Environment:

java version "11-ea" 2018-09-25
Java(TM) SE Runtime Environment 18.9 (build 11-ea+21)
Java HotSpot(TM) 64-Bit Server VM 18.9 (build 11-ea+21, mixed mode)
OS : Ubuntu


Approval: Vetted

 Description   

I was trying to build Clojure master on Travis against JDK 11 EA builds and got the below error . This is caused due to overloaded toArray and a similar [issue](https://dev.clojure.org/jira/browse/CRRBV-18) has been found at core.rrb-vector. The applied patch fixes the issue there and it seems that the change was introduced recently. Please refer to the [comment](https://dev.clojure.org/jira/browse/CRRBV-18?focusedCommentId=49545&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-49545) for the change.

Travis log : https://travis-ci.org/tirkarthi/clojure/jobs/402132377

I searched for JIRA using JDK 11 and Java 11 and couldn't come up with anything similar to this. Since this is an EA build I don't know if it will be added in the final release. Also if this going to be fixed will this mean that versions below master won't run on JDK 11 and above?

Thanks



 Comments   
Comment by Karthikeyan Singaravelan [ 10/Jul/18 5:23 AM ]

I applied the patch by Will Cohen for core.rrb-vector and Travis builds are green : https://travis-ci.org/tirkarthi/clojure/builds/402141339

Relevant commit : https://github.com/tirkarthi/clojure/commit/63dab8e6cb702a6b0c5b279721bee7eff0aba44f
Commit as patch : https://github.com/tirkarthi/clojure/commit/63dab8e6cb702a6b0c5b279721bee7eff0aba44f.patch

Credits to Will Cohen

Edit : The change seems to introduce source incompatibility

Ref : https://bugs.openjdk.java.net/browse/JDK-8060192?focusedCommentId=14194092&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14194092
Relevant commit : http://hg.openjdk.java.net/jdk/jdk11/rev/3ef0862bbb3d

Thanks

Comment by Alex Miller [ 10/Jul/18 7:43 AM ]

We added JDK 11 EA to our test build matrix yesterday but I had not had the chance to file this yet (https://build.clojure.org/job/clojure-test-matrix/), so thanks! I think it's worth looking into Compiler/Reflector changes to see if this is fixable at a different level.

I don't see that this means that this or other versions of Clojure would not work on Java 11. The bytecode produced is still compatible; this is an issue caused by a JDK lib change.

Comment by Ghadi Shayban [ 10/Jul/18 11:09 AM ]

That's right, AOT'ed bytecode will still work on JDK11, but you can't build gvec on JDK11 because there is a new overload to toArray that requires a type hint. We could add the disambiguation hint now in anticipation of the change.

Comment by Alex Miller [ 10/Jul/18 1:48 PM ]

I don't think it's a new overload - the arity existed, but now has a default implementation, right?

Comment by Ghadi Shayban [ 10/Jul/18 2:00 PM ]

That's right, it's a new method with the same name and arity, but different arg types. Users can override default interface methods, so from our perspective we need to have a type hint to disambiguate the new method when reifying:

toArray(T[] a)
toArray(IntFunction<T[]> generator) # added

(This method could have been added with a new name!)





[CLJ-2321] [spec] anonymous predicate in s/merge always fails Created: 01/Feb/18  Updated: 09/Jul/18

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

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


 Description   

user=> (require '[clojure.spec.alpha :as s])
nil
user=> (s/def ::a int?)
:user/a
user=> (s/def ::b int?)
:user/b
user=> (s/def ::c (s/merge (s/keys :req [::a ::b]) #(< (::a %) (::b %))))
:user/c
user=> (s/explain-data ::c {::a 0 ::b 1})
#:clojure.spec.alpha{:problems ({:path [], :pred (clojure.core/fn [%] (clojure.core/< (:user/a %) (:user/b %))), :val #:user{:a 0, :b 1}, :via [:user/c], :in []}), :spec :user/c, :value #:user{:a 0, :b 1}}
user=> ;; ^^ not expected
user=> (s/explain-data ::c {::a 1 ::b 0})
#:clojure.spec.alpha{:problems ({:path [], :pred (clojure.core/fn [%] (clojure.core/< (:user/a %) (:user/b %))), :val #:user{:a 1, :b 0}, :via [:user/c], :in []}), :spec :user/c, :value #:user{:a 1, :b 0}}
user=> ;; ^^ expected
user=> (s/def ::a<b #(< (::a %) (::b %)))
:user/a<b
user=> (s/def ::d (s/merge (s/keys :req [::a ::b]) ::a<b))
:user/d
user=> (s/explain-data ::d {::a 0 ::b 1})
nil
user=> (s/explain-data ::d {::a 1 ::b 0})
#:clojure.spec.alpha{:problems ({:path [], :pred (clojure.core/fn [%] (clojure.core/< (:user/a %) (:user/b %))), :val #:user{:a 1, :b 0}, :via [:user/d :user/a<b], :in []}), :spec :user/d, :value #:user{:a 1, :b 0}}




 Comments   
Comment by David Chelimsky [ 01/Feb/18 4:58 PM ]

It also always fails when referring to a predicate function, so the only way to get it to work is to define another spec and refer to that.

user=> (defn a<b [a b] (< a b))
#'user/a<b
user=> (s/def ::e (s/merge (s/keys :req [::a ::b]) a<b))
:user/e
user=> (s/explain-data ::e {::a 1 ::b 2})
#:clojure.spec.alpha{:problems ({:path [], :pred user/a<b, :val #:user{:a 1, :b 2}, :via [:user/e], :in []}), :spec :user/e, :value #:user{:a 1, :b 2}}
user=> (s/explain-data ::e {::a 1 ::b 0})
#:clojure.spec.alpha{:problems ({:path [], :pred user/a<b, :val #:user{:a 1, :b 0}, :via [:user/e], :in []}), :spec :user/e, :value #:user{:a 1, :b 0}}
Comment by Alex Miller [ 01/Feb/18 4:58 PM ]

#(< (::a % ::b %)) doesn't seem right? Does #(< (::a %) (::b %)) work?

Comment by David Chelimsky [ 01/Feb/18 5:01 PM ]

Good catch, however, no #(< (::a %) (::b %)) does not work inline. I updated the example with to reflect that.

Comment by David Chelimsky [ 01/Feb/18 5:02 PM ]

Also, FWIW, I'd narrowed down the problem elsewhere, before my typo Just so happened that I got the same answer with and without the typo.

Comment by David Chelimsky [ 01/Feb/18 5:15 PM ]

It does work if you wrap the predicate in {s/spec}:

user=> (s/def ::f (s/merge (s/keys :req [::a ::b]) (s/spec #(< (::a %) (::b %)))))
:user/f
user=> (s/explain-data ::f {::a 1 ::b 2})
nil
user=> (s/explain-data ::f {::a 1 ::b 0})
#:clojure.spec.alpha{:problems ({:path [], :pred (clojure.core/fn [%] (clojure.core/< (:user/a %) (:user/b %))), :val #:user{:a 1, :b 0}, :via [:user/f], :in []}), :spec :user/f, :value #:user{:a 1, :b 0}}
Comment by jcr [ 09/Jul/18 5:54 PM ]

I'm unable to reproduce any of this.

In the issue description, the predicate #(< (::a %) (::b %)) require a to be less than b, so it is expected that {::a 1 ::b 0} doesn't match the spec.

In the first comment, the predicate a<b is invalid, since it accepts two numbers instead of a single map.

The following snippet works as expected too (there's no difference between anonymous pred, named pred and a pred wrapped in s/spec):

(s/def ::a number?)
(defn good? [m] (contains? m :a))

(def s1 (s/merge (s/keys :req-un [::a]) good?))
(def s2 (s/merge (s/keys :req-un [::a]) #(contains? % :a)))
(def s3 (s/merge (s/keys :req-un [::a]) (s/spec #(contains? % :a))))

(s/valid? s1 {:a 42}) ;=> true
(s/valid? s2 {:a 42}) ;=> true 
(s/valid? s3 {:a 42}) ;=> true

I believe the issue can be closed.

Comment by David Chelimsky [ 09/Jul/18 7:27 PM ]

When I updated the example after Alex's comment, I put the message in the wrong place. I just updated it again with the messages correctly aligning w/ the output. jcr, I do see that your examples work, but mine still do not. So this is not quite as general as all anonymous predicates, but there is still surprising behavior.

That said, s/merge docs say it supports keys specs, and does not claim to support any other type of predicate, so if you want to close it on those grounds, have at it.

Comment by jcr [ 09/Jul/18 9:41 PM ]

Ah, now I see what you mean. Sorry for jumping to conclusions.

So the actual problem here is that s/explain inconsistently reports inputs that are valid according to s/conform when s/merge is used with a predicate not wrapped in a s/spec call. Here's the minimal example I've managed to come up with:

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

(defn pred [m] (contains? m :a)) 
(s/def ::s (s/merge pred))

(def good {:a 42})
(def bad  {})

;; as expected
(s/conform ::s good) ;=> {:a 42}
(s/conform ::s bad)  ;=> :clojure.spec.alpha/invalid

;; !!!: first one should NOT fail
(s/explain-str ::s good) ;=> "val: {:a 42} fails spec: :test/s predicate: pred\n" 
(s/explain-str ::s bad)  ;=> "val: {} fails spec: :test/s predicate: pred\n"

;; explicitly wrap pred in spec
(s/def ::s* (s/merge (s/spec pred)))

;; now, works as expected
(s/explain-str ::s* good) ;=> "Success!\n"
(s/explain-str ::s* bad)  ;=> "val: {} fails spec: :test/s* predicate: pred\n"

I would suggest updating the title and the description accordingly, if you don't mind?

Comment by jcr [ 09/Jul/18 10:25 PM ]

As far as I can tell, the problem is that merge-spec-impl simply calls explain-1 with the given preds, and explain-1 always returns problems if (spec? pred) is false; merge-spec-impl should probably call specize on its preds, similar to how it's done in the tuple-impl:

(let [specs (delay (mapv specize preds forms)) ...] ...)




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

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

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

Java 8


Attachments: Text File CLJ-2224-p1.patch    
Approval: Triaged

 Description   

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

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

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

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



 Comments   
Comment by Ghadi Shayban [ 26/Jun/18 11:11 PM ]

If we make Instants print as #inst like java.util.Date, it could change roundtripping (a read #inst is by default a j.u.Date)

This would be nice when Java 8 is soon baseline for Clojure.

Comment by Will Cohen [ 09/Jul/18 4:38 PM ]

CLJ-2224-p1.patch adds a print-method, print-dup, and read-instant-instant for java.time.Instant.





[CLJ-2372] spec/valid? and spec/explain inconsistent for coll-of :kind Created: 09/Jul/18  Updated: 09/Jul/18  Resolved: 09/Jul/18

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

Type: Defect Priority: Major
Reporter: Dale Thatcher Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: spec
Environment:

Windows XP, MacOS 10.13, JDK 1.8.0_171, Clojure 1.9.0, clojure.spec.alpha



 Description   

valid? and explain-str give inconsistent results when using coll-of :kind with a registered spec.

(s/def ::vector? vector?)
=> :user/vector?
(s/def ::vector-of-maps (s/coll-of map? :kind ::vector?))
=> :user/vector-of-maps
(s/valid? ::vector-of-maps [{}])
=> false
(s/explain-str ::vector-of-maps [{}])
=> "Success!\n"



 Comments   
Comment by Alex Miller [ 09/Jul/18 8:24 AM ]

It is not valid to use a spec for :kind.

Comment by Dale Thatcher [ 09/Jul/18 8:34 AM ]

Ok, the doc string doesn't match then:

coll-of refers you to "every" which has:

:kind - a pred/spec that the collection type must satisfy, e.g. vector?
(default nil) Note that if :kind is specified and :into is
not, this pred must generate in order for every to generate."

Also I noticed that the documentation for coll-of specifies:

Returns a spec for a collection of items satisfying pred.

But in this case a spec can be used.

Comment by Alex Miller [ 09/Jul/18 9:03 AM ]

The doc was fixed in the newest release of spec.alpha (0.2.168) - see CLJ-2111.

Comment by Dale Thatcher [ 09/Jul/18 9:57 AM ]

Understood, sorry to have wasted your time. I'll check the latest source next time, before raising a Jira.

Comment by Alex Miller [ 09/Jul/18 10:00 AM ]

No worries at all! That’s what triage is all about.





[CLJ-2159] Disambiguate behavior of def with doc-string Created: 23/Apr/17  Updated: 09/Jul/18

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

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

REPL


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

 Description   

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

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



 Comments   
Comment by jcr [ 06/Jul/18 10:09 AM ]

This page is among the first google results for "clojure def docstring". Since it's a trivial documentation improvement, can it be merged for the next release?

Comment by Nicola Mometto [ 07/Jul/18 5:06 AM ]

FYI you can just do `(def ^{:doc "my-doc-here} var)`, altho for this use case `declare` should be preferred.

Comment by jcr [ 07/Jul/18 6:19 PM ]

Nicola, right; strictly speaking, the "it's impossible to use `def` to create a var that's unbound but has `:doc` metadata" bit of the issue description is incorrect, but that actually doesn't affect the correctness and usefullness of the change proposed in the patch itself. Just for clarification, this :forms meta actually reflects how def is implemented (while the current one does not):

[(def symbol) (def symbol doc-string? init)]
Comment by Nicola Mometto [ 09/Jul/18 7:48 AM ]

Definitely! I wasn't contesting this ticket, just letting you know you can use the explicit metadata syntax, in case you weren't aware of it (as the description seemed to suggest)





[CLJ-2371] [spec] alt with an empty cat breaks explain Created: 06/Jul/18  Updated: 08/Jul/18

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

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

Approval: Vetted

 Description   
(s/conform (s/alt :empty (s/cat)) [42]) ; => ::s/invalid
(s/explain (s/alt :empty (s/cat)) [42]) ; => prints 'Success!'

Related:
CLJ-2360 - looks like the same issue, but for s/and, s/or
CLJ-2336 - looks like a consequence of this issue
CLJ-2304 - probably related too






[CLJ-1966] [spec] :clojure.spec/invalid is not a valid :clojure.spec/any value Created: 21/Jun/16  Updated: 06/Jul/18

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: 6
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





[CLJ-2358] read+string does not support reader conditionals Created: 12/Jun/18  Updated: 05/Jul/18

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

Type: Enhancement Priority: Minor
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reader, readerconditionals

Approval: Triaged

 Description   

Because read+string calls downwards with (apply read rdr args) it does not support reader conditionals. Those have to be flagged in through the opts parameter to #'read, which is only available with the oddly backwards signature (read opts rdr)



 Comments   
Comment by Nicola Mometto [ 15/Jun/18 10:21 AM ]

Here's the implementation of tools.reader, which doesn't contain this bug: https://github.com/clojure/tools.reader/blob/master/src/main/clojure/clojure/tools/reader.clj#L1013-L1015
Something similar could be done here, or read+string could be manually unrolled

Comment by Ghadi Shayban [ 05/Jul/18 12:20 PM ]

read+string doesn't reach the underlying (read opts stream) signature at all, so whatever missing opts are not limited to reader conditionals





[CLJ-2367] case with byte value causes CompilerException Created: 27/Jun/18  Updated: 04/Jul/18  Resolved: 04/Jul/18

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

Type: Defect Priority: Critical
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: compiler

Attachments: Text File 0001-CLJ-2367-Incorporate-ASM-merge-request-189-and-add-c.patch     Text File 0001-Fix-CLJ-2367-by-restoring-no-op-cast-from-SHORT-BYTE.patch    
Approval: Ok

 Description   

Summary: The Clojure ASM upgrade exposed a regression in ASM. This patch matches the patch being applied to ASM.

Approach: Copy the patch being made to ASM https://gitlab.ow2.org/asm/asm/commit/7d045e01cdadad95d62534ef92cb0eca2eec1a17 plus add test

Patch: 0001-CLJ-2367-Incorporate-ASM-merge-request-189-and-add-c.patch

Repro
Clojure 1.10.0-alpha5 fails as follows:

(! 803)-> clj -A:master
Downloading: org/clojure/clojure/1.10.0-master-SNAPSHOT/maven-metadata.xml from https://oss.sonatype.org/content/repositories/snapshots/
Clojure 1.10.0-master-SNAPSHOT
user=> (case (byte 100) 1 2 3)
CompilerException java.lang.IllegalArgumentException, compiling:(NO_SOURCE_PATH:1:1) 
user=> (pst)
CompilerException java.lang.IllegalArgumentException, compiling:(NO_SOURCE_PATH:1:1)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:7010)
	clojure.lang.Compiler.analyze (Compiler.java:6773)
	clojure.lang.Compiler.eval (Compiler.java:7059)
	clojure.lang.Compiler.eval (Compiler.java:7025)
	clojure.core/eval (core.clj:3206)
	clojure.core/eval (core.clj:3202)
	clojure.main/repl/read-eval-print--8730/fn--8733 (main.clj:243)
	clojure.main/repl/read-eval-print--8730 (main.clj:243)
	clojure.main/repl/fn--8739 (main.clj:261)
	clojure.main/repl (main.clj:261)
	clojure.main/repl-opt (main.clj:325)
	clojure.main/main (main.clj:424)
Caused by:
IllegalArgumentException 
	clojure.asm.commons.GeneratorAdapter.cast (GeneratorAdapter.java:785)
	clojure.lang.Compiler$CaseExpr.emitExprForInts (Compiler.java:8805)
	clojure.lang.Compiler$CaseExpr.doEmit (Compiler.java:8728)
	clojure.lang.Compiler$CaseExpr.emit (Compiler.java:8705)
	clojure.lang.Compiler$BodyExpr.emit (Compiler.java:6144)
	clojure.lang.Compiler$LetExpr.doEmit (Compiler.java:6503)
	clojure.lang.Compiler$LetExpr.emit (Compiler.java:6453)
	clojure.lang.Compiler$BodyExpr.emit (Compiler.java:6144)
	clojure.lang.Compiler$ObjMethod.emitBody (Compiler.java:5844)
	clojure.lang.Compiler$FnMethod.doEmitStatic (Compiler.java:5508)
	clojure.lang.Compiler$FnMethod.emit (Compiler.java:5473)
	clojure.lang.Compiler$FnExpr.emitMethods (Compiler.java:3937)
nil

Screening Notes: I followed all the discussion/links and concur with the assessment, and that this patch matches the fix being applied to ASM.



 Comments   
Comment by Sean Corfield [ 27/Jun/18 10:20 PM ]

Compiler.java, line 8812: gen.cast(exprType, Type.INT_TYPE); used to be a no-op for INT, SHORT, BYTE and now it throws an exception for SHORT and BYTE.

There only appear to be the only two calls to `gen.cast()` (lines 8812 and 8845). The latter casts to LONG so that is still covered by the new ASM code.

If line 8812 was if (exprType == Type.LONG_TYPE) gen.cast(exprType, Type.INT_TYPE); then the previous behavior would be restored.

Comment by Sean Corfield [ 27/Jun/18 10:44 PM ]

Patch to avoid cast from SHORT/BYTE to INT (which is a no-op). Includes tests.

Comment by daniel sutton [ 27/Jun/18 11:15 PM ]

Just wanted to add some background:
This was a change in the ASM library at https://gitlab.ow2.org/asm/asm/commit/d493987d57b981b21ba97d4226ee7b94b0dc8406 with commit message "Reformat the source code with google-java-format 1.3".

The cast method did not throw an error and after a reformatting it now does. I think this would be a good candidate for them to fix and I am in the process of opening an issue on their gitlab instance. It takes some time due to spam filtering. (File before "reformat" commit: https://gitlab.ow2.org/asm/asm/blob/d493987d57b981b21ba97d4226ee7b94b0dc8406/src/org/objectweb/asm/commons/GeneratorAdapter.java#L714)

This seems like a good candidate for an upstream fix for a few reasons:
1. this is a no-op cast and seems like the library should handle this quite possible cast without pushing onto callers which casts are possible but invalid
2. the change occurred in a reformat commit which makes me think that the change is not apparent to the maintainers. This seems to be an automatically generated code in a commit that touched 262 files.
3. It introduces more branching and seemingly strange checks in the Clojure compiler. If the library is unresponsive or intends this to be an error it makes sense but it seems like trying to get this fixed there could be valuable for us and others.

Comment by Alex Miller [ 27/Jun/18 11:39 PM ]

Wow, really? If it does turn out to be an error in ASM, we revendor it in our code and can just change our version (but should also provide that fix back to upstream of course).

Comment by daniel sutton [ 28/Jun/18 9:10 PM ]

Ticket opened on OW2 asm project: https://gitlab.ow2.org/asm/asm/issues/317837

Comment by Ghadi Shayban [ 28/Jun/18 10:48 PM ]

daniel sutton: the change did not happen unintentionally in ASM in d493987d57b981b21ba97d4226ee7b94b0dc8406 (reformat code) but in a4b16ab323945c83e1d705997f00c95340e1bc2e [1] [2]

[1] https://gitlab.ow2.org/asm/asm/commit/a4b16ab323945c83e1d705997f00c95340e1bc2e
[2] https://gitlab.ow2.org/asm/asm/merge_requests/135/diffs

Comment by Sean Corfield [ 28/Jun/18 11:49 PM ]

It still looks like a random and unnecessary change in ASM. There's no test for the exception case, there's no open issue being closed by either of those merges that relates to that cast() issue. I'm a bit surprised it got through the review without someone asking "Hey, so why are we disallowing this case?"..

Comment by Alex Miller [ 29/Jun/18 9:17 AM ]

Could be some kind of tightening in accordance with the jvm spec. But would be nice if there was a note about that if so.

Comment by daniel sutton [ 30/Jun/18 11:28 AM ]

from the ow2 ticket:

> Thanks for your report. The regression was actually introduced by a4b16ab3, and will be fixed with !189.

https://gitlab.ow2.org/asm/asm/merge_requests/189

Comment by Alex Miller [ 30/Jun/18 2:39 PM ]

Would be great if someone could make a patch in the Clojure equivalent version of this code and test...

Comment by Sean Corfield [ 30/Jun/18 8:33 PM ]

Sure, I'll update my patch to include the changes from merge_requests/189, revert the Compiler.java change, and still include the new tests.

Comment by Sean Corfield [ 30/Jun/18 8:36 PM ]

Updated patch: preserves new case tests, reverts Compiler.java change, incorporates GeneratorAdapter.java changes from their merge request 189.

Comment by daniel sutton [ 03/Jul/18 1:27 PM ]

asm commit included fix and tests: https://gitlab.ow2.org/asm/asm/commit/58d73da3176e39e3416d3e8cddff25266258542f





[CLJ-2370] fold should (optionally) warn on sequential processing Created: 02/Jul/18  Updated: 02/Jul/18

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

Type: Enhancement Priority: Minor
Reporter: JAre Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers, warnings


 Description   

It would be great to have a flag similar to warn-on-reflection but for fold to warn when it reduces sequentially (not parallel).



 Comments   
Comment by JAre [ 02/Jul/18 9:14 AM ]

Not sure what are requirements for fold to be parallelized but if it's associativity of the collection then the issue can be resolved by mentioning "associative?" predicate in the fold's doc-string.





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

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

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

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

 Description   

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

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

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

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



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

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

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

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

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

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

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

Thanks for the report!

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

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

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

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

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

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

Comment by Ben Bader [ 09/Feb/18 4:54 PM ]

Hi, is there anything I can or should to to move this forward?

Comment by Alex Miller [ 12/Feb/18 8:47 AM ]

Nope. We batch up tickets to look at them periodically.





[CLJ-2368] [spec] describe* of spec-impl returns nil :args and :fn Created: 29/Jun/18  Updated: 29/Jun/18

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

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

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

 Description   

The form of an empty fspec returns nil at :args and :fn:

(s/form (s/fspec))
=> (clojure.spec.alpha/fspec :args nil :ret clojure.core/any? :fn nil)

The problem is, that the specs from CLJ-2112 don't validate that form. I suggest to implement describe* of fspec-impl like that of map-spec-impl which omits nil values.

Patch: clj-2368.patch



 Comments   
Comment by Alex Miller [ 29/Jun/18 8:55 AM ]

Sounds right.





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

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

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

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

 Description   

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

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

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

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

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

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

Patch: clj-1435.patch

Screened by: Alex Miller

Questions from screening for Rich:

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



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

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

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

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

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

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

Also consider:

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

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

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

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

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

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

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

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

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

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

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

Any progress on this?

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

Sitting in the queue, waiting for love.

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

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

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

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





[CLJ-2044] Four functions in clojure.instant have incomplete documentation Created: 15/Oct/16  Updated: 29/Jun/18

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

Type: Enhancement Priority: Major
Reporter: Bruce Adams Assignee: Bruce Adams
Resolution: Unresolved Votes: 0
Labels: docstring, instant

Attachments: Text File defns-for-instant-def-timestamp.patch     Text File defns-for-instant.patch    
Patch: Code
Approval: Screened

 Description   

Of the five public functions defined in clojure.instant, these four:

  • parse-timestamp
  • read-instant-calendar
  • read-instant-date
  • read-instant-timestamp

are each declared as a Var without any arglists metadata. This means documentation does not contain function calling information.

In http://clojure.github.io/clojure/clojure.instant-api.html, each of these functions is described as a var and there is no Usage: ... information given.

The output of doc does not include argument list information. For example:

user=> (doc clojure.instant/read-instant-date)
-------------------------
clojure.instant/read-instant-date
  To read an instant as a java.util.Date, bind *data-readers* to a map with
this var as the value for the 'inst key. The timezone offset will be used
to convert into UTC.

A related problem is that stack traces show anonymous functions instead of the names for any of these functions. For example:

user=> (clojure.instant/read-instant-timestamp "123")
RuntimeException Unrecognized date/time syntax: 123  clojure.instant/fn--6879/fn--6880 (instant.clj:107)

Proposed: Refactor the code into defn functions which makes the code clearer and addresses the documentation issue. An alternate approach would be to apply :arglists metadata to the vars.

Patch: defns-for-instant-def-timestamp.patch
Screened: Alex Miller



 Comments   
Comment by Bruce Adams [ 15/Oct/16 12:40 PM ]

Proposed solution: refactor the definitions of the four problematic functions to be defined using defn.

Comment by Bruce Adams [ 16/Oct/16 5:24 PM ]

Some of my thinking leading to the solution I propose.

Initially, when I realized that I didn't know what arguments parse-timestamp required, I assumed the appropriate fix was to enhance the docstring. Then I noticed that the on-line documentation for functions was formatted quite differently from the text output by doc. Any decent fix was going to have to feed function information into different variants of documentation formatting code.

I can guess, from other examples such as first, that :arglists metadata is what indicates that a var is a function. One possible solution would be to add :arglists to each of the four functions. It felt cleaner to refactor the code into simple defn functions. Refactoring code just for the side effect of documentation seems a bit odd, but the code itself strikes me as more legible after my refactoring.

Comment by Alex Miller [ 17/Oct/16 9:53 AM ]

This seems reasonable to me. I would move the timestamp regex into a separate (private) var instead of creating it in parse-timestamp.

It's possible the way these functions were defined was designed to minimize startup time or something like that, but I don't have any background on that.

Comment by Bruce Adams [ 23/Oct/16 4:06 PM ]

Updated patch based on Alex's great suggestion. This adds a separate, private, def for the timestamp regex.





[CLJ-2249] Document behavior of clojure.core/get on strings and Java arrays Created: 05/Oct/17  Updated: 29/Jun/18

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

Type: Enhancement Priority: Minor
Reporter: Arne Brasseur Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File 0001-Expand-docstring-of-clojure.core-get.patch     Text File CLJ-2249-patch-2.patch    
Patch: Code
Approval: Screened

 Description   

get's implementation checks in order for

  • ILookup
  • nil
  • Map
  • IPersistentSet
  • String or Java array

The docstring for get currently reads

"Returns the value mapped to key, not-found or nil if key not present."

That this works on maps and associative data can reasonably be inferred if one knows Clojure's data model. That it works on sets, Strings, and arrays is less obvious, and would be helpful to mention.

Patch: CLJ-2249-patch-2.patch
Screened by: Alex Miller



 Comments   
Comment by Arne Brasseur [ 05/Oct/17 12:49 PM ]

I've tried to stick to the same terse style, happy to take suggestions for phrasing. I also changed the first sentence slightly, because I found it difficult to parse, but that is an unrelated change that I can undo if that's preferred.

Comment by Alex Miller [ 05/Oct/17 1:14 PM ]

I'd leave the first sentence as is.

All of these collection ops are kind of tricky in being able to succinctly state the intent, while also covering special cases (which are often related to Java types). Here I think the intent is to cover lookup in "associative data structures" which covers Clojure maps, records, vectors, Java maps, and other less obvious things like weird ILookup impls.

The non-obvious inclusions for me are: Clojure sets (I haven't reviewed but undoubtedly this is implicitly used in a bunch of special cases), and the Java special cases which are Strings and arrays. For an example of wording, I would point to `count` and `nth`, which are similarly weird.

So maybe a sentence like: "get also works on sets to return contained values, and on strings and arrays for value by index." ?

We'll need to answer these same questions in the spec too btw. I expect the act of spec'ing core fns to drive more of these tricky questions.

Comment by David Liepmann [ 05/Oct/17 2:18 PM ]

>I'd leave the first sentence as is.

Would a rephrase be welcome in its own issue? Right now the referent of "the value mapped to" is ambiguous. I agree it's difficult to parse.

Comment by Alex Miller [ 05/Oct/17 2:50 PM ]

David Liepmann no thanks

Comment by Arne Brasseur [ 09/Oct/17 3:27 AM ]

Appended new patch.





[CLJ-1550] Classes generated by deftype and defrecord don't play nice with .getPackage Created: 07/Oct/14  Updated: 29/Jun/18

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

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 15
Labels: classloader, deftype

Attachments: Text File 0001-CLJ-1550-define-package-for-class-in-DynamicClassLoa.patch     Text File CLJ-1550-v2.patch     Text File clj-1550-v3.patch     Text File clj-1550-v4.patch    
Patch: Code and Test
Approval: Screened

 Description   

Classes generated loaded by DynamicClassLoader return nil for .getPackage. Tools like CIDER and vim-fireplace are relying on this information to implement things like completion hints.

(.getPackage String)
;; => #<Package package java.lang, Java Platform API Specification, version 1.7>
(deftype T [])
(.getPackage T)
;; => nil

Proposed: During DynamicClassLoader.defineClass(), invoke definePackage() on the class being defined (similar to what URLClassLoader does).

Patch: clj-1550-v4.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 07/Oct/14 8:54 AM ]

According to http://docs.oracle.com/javase/7/docs/api/java/lang/Class.html#getPackage() this method returns the package information found by the class loader or null if there is none. Its not clear to me that the current behavior is wrong per the spec. I would need to experiment more to see if this is unusual or not.

Comment by Bozhidar Batsov [ 07/Oct/14 9:05 AM ]

A bit of background for the issue. I'm no expert on the topic, but being able to procure all the class information except its package definitely looks strange to me.

Comment by Kevin Downey [ 07/Oct/14 11:46 AM ]

if you AOT compile(generate a class file on disk for a deftype), getPackage works fine, which suggests to me it is a jvm issue

Comment by Kevin Downey [ 07/Oct/14 11:49 AM ]

actually, it must just be that dynamicclassloader doesn't define a package for classes it loads

Comment by Alex Miller [ 07/Oct/14 12:13 PM ]

Yep, I believe that's correct.

Comment by Stuart Halloway [ 21/Jul/15 8:01 AM ]

There is no problem statement here. What is package information needed for?

Comment by Bozhidar Batsov [ 21/Jul/15 8:05 AM ]

I've linked the problem above. Basically tools like CIDER and vim-fireplace are relying on this information to implement things like completion hints.
This might not be problem when running your apps, but it's definitely a problem when inspecting their state...

Comment by Michael Blume [ 22/Jul/15 12:32 PM ]

s/Packate/Package

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

Refreshed patch to apply to current master, attribution retained, no semantic changes. Marked prescreened.

Comment by Ghadi Shayban [ 26/Jun/18 10:55 PM ]

We should consider interactions with the JDK9 module system here

Comment by Alex Miller [ 26/Jun/18 11:28 PM ]

off the top of my head, I don't see how this would do anything negative wrt the JDK9 module system

Comment by Alex Miller [ 29/Jun/18 11:00 AM ]

v4 patch rebases to master and retains attribution, no semantic changes.

Comment by Bozhidar Batsov [ 29/Jun/18 4:18 PM ]

Alex, is this going to be included in Clojure 1.10?

Comment by Alex Miller [ 29/Jun/18 5:31 PM ]

That's the current path...





[CLJ-2369] clojure.test/is + thrown-with-msg? error when caught exception message is nil Created: 29/Jun/18  Updated: 29/Jun/18

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

Type: Defect Priority: Minor
Reporter: Yuri Govorushchenko Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: clojure.test, error-reporting
Environment:

Clojure 1.9.0, CLJS 1.10.238


Approval: Triaged

 Description   

Steps:

(require '[clojure.test :refer [is]])
(is (thrown-with-msg? Exception #"123" (throw (ex-info nil {}))))

Expected:

FAIL in () (form-init1933636226458515246.clj:1)
expected: (thrown-with-msg? Exception #"123" (throw (ex-info nil {})))
  actual: ...

Actual:

ERROR in () (Matcher.java:1283)
expected: (thrown-with-msg? Exception #"123" (throw (ex-info nil {})))
  actual: java.lang.NullPointerException: null
 at java.util.regex.Matcher.getTextLength (Matcher.java:1283)
    java.util.regex.Matcher.reset (Matcher.java:309)
    java.util.regex.Matcher.<init> (Matcher.java:229)
    java.util.regex.Pattern.matcher (Pattern.java:1093)
    clojure.core$re_matcher.invokeStatic (core.clj:4796)
    clojure.core$re_find.invokeStatic (core.clj:4838)
    clojure.core$re_find.invoke (core.clj:4838)
    user$eval43312.invokeStatic (form-init1933636226458515246.clj:1)
    user$eval43312.invoke (form-init1933636226458515246.clj:1)
    clojure.lang.Compiler.eval (Compiler.java:7062)
    clojure.lang.Compiler.eval (Compiler.java:7025)
    clojure.core$eval.invokeStatic (core.clj:3206)
    clojure.core$eval.invoke (core.clj:3202)
    clojure.main$repl$read_eval_print__8572$fn__8575.invoke (main.clj:243)
    clojure.main$repl$read_eval_print__8572.invoke (main.clj:243)
    clojure.main$repl$fn__8581.invoke (main.clj:261)
    clojure.main$repl.invokeStatic (main.clj:261)
    clojure.main$repl.doInvoke (main.clj:177)
    clojure.lang.RestFn.invoke (RestFn.java:1523)
    clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__40865.invoke (interruptible_eval.clj:87)
    clojure.lang.AFn.applyToHelper (AFn.java:152)
    clojure.lang.AFn.applyTo (AFn.java:144)
    clojure.core$apply.invokeStatic (core.clj:657)
    clojure.core$with_bindings_STAR_.invokeStatic (core.clj:1965)
    clojure.core$with_bindings_STAR_.doInvoke (core.clj:1965)
    clojure.lang.RestFn.invoke (RestFn.java:425)
    clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invokeStatic (interruptible_eval.clj:85)
    clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invoke (interruptible_eval.clj:55)
    clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn__40910$fn__40913.invoke (interruptible_eval.clj:222)
    clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__40905.invoke (interruptible_eval.clj:190)
    clojure.lang.AFn.run (AFn.java:22)
    java.util.concurrent.ThreadPoolExecutor.runWorker (ThreadPoolExecutor.java:1149)
    java.util.concurrent.ThreadPoolExecutor$Worker.run (ThreadPoolExecutor.java:624)
    java.lang.Thread.run (Thread.java:748)

ClojureScript:

ERROR in () (TypeError:NaN:NaN)
expected: (thrown-with-msg? js/Error #"123" (throw (ex-info nil {})))
  actual: #object[TypeError TypeError: re-find must match against a string.]





[CLJ-1218] mapcat is too eager Created: 16/Jun/13  Updated: 29/Jun/18

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

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

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

 Description   

The following expression prints 1234 and returns 1:

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

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

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

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


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

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

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

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

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

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

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

Updated join code to be actually valid.

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

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

(defn join
  "Lazily concatenates a sequence-of-sequences into a flat sequence."
  [s]
  (lazy-seq
   (when-let [s (seq s)] 
     (concat (first s) (join (rest s))))))
Comment by Ghadi Shayban [ 02/May/17 10:47 AM ]

Though the docstring makes no lazyiness promises (except declaring its specific implementation), this seems like a defect, not an enhancement. mapcat realizes 4 underlying collections at minimum:

boot.user=> (defn notifying-seq [cb!] (lazy-seq (cb!) (cons :ignored (notifying-seq cb!))))                                                              

#'boot.user/notifying-seq                                                                                                                                

boot.user=> (let [a (atom 0)] 
  (seq (mapcat (constantly [:ignored :ignored])
               (notifying-seq #(swap! a inc))))
  @a)
4




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

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

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

All


Attachments: Text File 0001-CLJ-2122-Have-flatten-docstring-describe-lazy-result.patch    
Patch: Code
Approval: Screened

 Description   

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

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

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

Screened by: Alex Miller



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

Seems reasonable.





[CLJ-2050] Remove redundant key comparisons in HashCollisionNode Created: 03/Nov/16  Updated: 29/Jun/18

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

Type: Enhancement Priority: Minor
Reporter: Kwang Yul Seo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, performance

Attachments: Text File 0001-Remove-redundant-key-comparisons-in-HashCollisionNod.patch    
Patch: Code
Approval: Screened

 Description   

Comparing key to array[idx] is redundant as findIndex already performed key comparison to find the index.

Screened by: Alex Miller






[CLJ-2306] Remove unused Var.rev Created: 03/Jan/18  Updated: 29/Jun/18

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

Type: Enhancement Priority: Minor
Reporter: Nathan Fisher Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: var
Environment:

all


Attachments: Text File remove-rev.patch    
Patch: Code
Approval: Screened

 Description   

Var.rev is unused. The usage that does exist is not synchronised correctly.

Alex suggested it be removed in this discussion;

https://groups.google.com/forum/#!topic/clojure-dev/idsFYAKnTPc

The attached patch removes rev from Var.

Screened: Alex Miller






[CLJ-1366] The empty map literal is read as a different map each time Created: 01/Mar/14  Updated: 29/Jun/18

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

Type: Enhancement Priority: Major
Reporter: Shogo Ohta Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: memory, reader

Attachments: Text File 0001-make-the-reader-return-the-same-empty-map-when-it-re.patch     Text File 0002-make-the-reader-return-the-same-empty-map-when-it-re.patch    
Patch: Code
Approval: Screened

 Description   

As reported here (https://groups.google.com/forum/?hl=en#!topic/clojure-dev/n83hlRFsfHg), the empty map literal is read as a different map each time.

user=> (identical? (read-string "{}") (read-string "{}"))
false

Making the reader return the same empty map when it reads an empty map is expected to improve some memory efficiency, and also lead to consistency with the way other collection literals are read in.

user=> (identical? (read-string "()") (read-string "()"))
true
user=> (identical? (read-string "[]") (read-string "[]"))
true
user=> (identical? (read-string "#{}") (read-string "#{}"))
true

Cause: LispReader calls RT.map() with an empty array when it reads an empty map, and RT.map() in turn makes a new map unless its argument given is null.

Approach: make RT.map() return the same empty map when the argument is an empty array as well, not only when null

Patch: 0002-make-the-reader-return-the-same-empty-map-when-it-re.patch

Screened by: Alex Miller



 Comments   
Comment by Shogo Ohta [ 01/Mar/14 2:59 AM ]

Sorry, the patch 0001-make-the-reader-return-the-same-empty-map-when-it-re.patch didn't work.

The updated patch 0002-make-the-reader-return-the-same-empty-map-when-it-re.patch works, but I'm afraid it'd be beyond the scope of this ticket since it modifies RT.map() behavior a bit.





[CLJ-1654] Reuse seq in some Created: 04/Feb/15  Updated: 29/Jun/18

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

Type: Enhancement Priority: Trivial
Reporter: Gijs Stuurman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ft, performance

Attachments: Text File 0000-reuse-seq-in-some.patch     Text File clj-1654.patch    
Patch: Code
Approval: Screened

 Description   

By using when-let at most two seq constructions can be avoided per invocation of some.

Patch: clj-1654.patch

Screened by: Alex Miller



 Comments   
Comment by Ghadi Shayban [ 04/Feb/15 12:11 PM ]

This is similar to the tweak to dorun/doall in CLJ-1515. It is a good benefit when a collection doesn't cache its seq

Comment by Alex Miller [ 12/Jan/16 9:40 AM ]

Updated patch to include ticket number in commit message, no other changes, attribution retained.





[CLJ-1668] ns macro throws NPE if empty reference is specified Created: 02/Mar/15  Updated: 29/Jun/18  Resolved: 29/Jun/18

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

Type: Enhancement Priority: Minor
Reporter: Philipp Meier Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: errormsgs, ft, macro, namespace

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

 Description   

The following invocations of `ns` will all throw a NPE

(ns foo ())
(ns foo [])
(ns foo (:require clojure.core) ())

;; throw

1. Unhandled java.lang.NullPointerException
   (No message)

                      core.clj: 1518  clojure.core/name
                      core.clj: 5330  clojure.core/ns/process-reference
                      core.clj: 2559  clojure.core/map/fn
                  LazySeq.java:   40  clojure.lang.LazySeq/sval
                  LazySeq.java:   49  clojure.lang.LazySeq/seq
                       RT.java:  484  clojure.lang.RT/seq
                      core.clj:  133  clojure.core/seq
                      core.clj:  694  clojure.core/concat/cat/fn
                  LazySeq.java:   40  clojure.lang.LazySeq/sval
                  LazySeq.java:   49  clojure.lang.LazySeq/seq
                     Cons.java:   39  clojure.lang.Cons/next
                       RT.java: 1654  clojure.lang.RT/boundedLength
                      AFn.java:  148  clojure.lang.AFn/applyToHelper
                      Var.java:  700  clojure.lang.Var/applyTo

I'd expect an exception that is describing the cause of the error, not an "symptom".

Proposed: Check for nil reference in ns and throw.

Patch: clj-1668.patch



 Comments   
Comment by Alex Miller [ 29/Jun/18 10:52 AM ]

These all throw spec errors now





[CLJ-1797] Mention cljc when require fails Created: 10/Aug/15  Updated: 29/Jun/18

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: Cezary Kosko
Resolution: Unresolved Votes: 1
Labels: errormsgs

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

 Description   

If you attempt to require a namespace for which the code cannot be located, now that cljc files are considered when locating code, it could be mentioned in the list of files considered on the classpath.

$ java -jar clojure-1.7.0.jar 
Clojure 1.7.0
user=> (require 'foo.bar)
FileNotFoundException Could not locate foo/bar__init.class or foo/bar.clj on classpath.  clojure.lang.RT.load (RT.java:449)

Note: FWIW, ClojureScript has similar error messages, and the order listed in the error message is the order tried when loading. If this were followed, I suspect the text of the error message above would end up looking like:

Could not locate foo/bar__init.class, foo/bar.clj, or foo/bar.cljc on classpath.

Patch: clj-1797.patch
Approach: add info about cljcfile not found in exception thrown from load
Screened by: Alex Miller



 Comments   
Comment by Cezary Kosko [ 18/Jan/16 6:21 AM ]

Hey,

I'm a newbie, so I thought I'd do that one (even though it's not vetted yet, but seems it 's bound to be), to get a tad more familiar w/ the sources.

Kind regards,
Cezary

Comment by Alex Miller [ 18/Jan/16 6:53 AM ]

Go for it!





[CLJ-1209] clojure.test does not print ex-info in error reports Created: 11/May/13  Updated: 29/Jun/18

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

Type: Enhancement Priority: Major
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: clojure.test

Attachments: Text File 0001-use-new-printing-method.patch     Text File 0002-CLJ-1209-show-ex-data-in-clojure-test.patch     File clj-test-print-ex-data.diff     Text File output-with-0002-patch.txt    
Patch: Code
Approval: Screened

 Description   

clojure.test does not print the data attached to ExceptionInfo in error reports.

(use 'clojure.test)
(deftest ex-test (throw (ex-info "err" {:some :data})))
(ex-test)

ERROR in (ex-test) (core.clj:4591)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ExceptionInfo: err
 at clojure.core$ex_info.invoke (core.clj:4591)
    user/fn (NO_SOURCE_FILE:2)
    clojure.test$test_var$fn__7666.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)
    ...

Approach: In clojure.stacktrace, which clojure.test uses for printing exceptions, add a check for ex-data and pr it.

After:

ERROR in (ex-test) (core.clj:4591)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ExceptionInfo: err
{:some :data}
 at clojure.core$ex_info.invoke (core.clj:4591)
    user/fn (NO_SOURCE_FILE:3)
    clojure.test$test_var$fn__7667.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)

Patch: 0002-CLJ-1209-show-ex-data-in-clojure-test.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 20/Dec/13 9:53 AM ]

Great idea, thx for the patch!

Comment by Alex Miller [ 20/Dec/13 9:54 AM ]

Would be great to see a before and after example of the output.

Comment by Ivan Kozik [ 12/Jul/14 10:35 PM ]

Attaching sample output

Comment by Stuart Sierra [ 05/Sep/14 3:24 PM ]

As pointed out on IRC, there's a possible risk of trying to print an infinite lazy sequence that happened to be included in ex-data.

To mitigate, consider binding *print-length* and *print-level* to small numbers around the call to pr.

Comment by Stephen C. Gilardi [ 13/May/15 2:39 PM ]

http://dev.clojure.org/jira/browse/CLJ-1716 may cover this well enough that this issue can be closed.

Comment by Alex Miller [ 14/May/15 8:35 AM ]

I don't think 1716 covers it at all as clojure.test/clojure.stacktrace don't use the new throwable printing. But they could! And that might be a better solution than the patch here.

For example, the existing patch does not consider what to do about nested exceptions, some of which might have ex-data. The new printer handles all that in a consistent way.

Comment by Ed Bowler [ 22/Dec/16 11:35 AM ]

I think http://dev.clojure.org/jira/secure/attachment/16361/0001-use-new-printing-method.patch fixes the printing of the Exceptions.





[CLJ-2112] [spec] Add specs for spec forms Created: 15/Feb/17  Updated: 28/Jun/18

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

Type: Feature Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 23
Labels: spec

Attachments: Text File spec-forms.patch    
Approval: Incomplete

 Description   

It would be useful to have specs that described spec forms, such that it was possible to go from a spec form like (s/keys :req [::a ::b] :opt [::c]) to a conformed version that allowed you to grab the parts without parsing the s-expression. This can be done by creating specs, thus allowing:

user=> (require '[clojure.spec :as s] '[clojure.spec.specs])
user=> (s/def ::aspec (s/keys :req [::a ::b] :opt [::c]))
user=> (def aspec-data (s/conform :clojure.spec.specs/spec (s/form ::aspec)))
user=> (pr aspec-data)
[:form {:s clojure.spec/keys, 
        :args {:req [[:key :clojure.spec.specs/a] [:key :clojure.spec.specs/b]], 
               :opt [:clojure.spec.specs/c]}}]
user=> (map val (-> aspec-data val :args :req))
(:clojure.spec.specs/a :clojure.spec.specs/b)

Patch: spec-forms.patch (a work in progress)



 Comments   
Comment by Saul Shanabrook [ 05/Aug/17 6:08 PM ]

Could I help out on this? Happy to work on it. It would be very helpful for me, trying parse the the specs from the :args and :ret in fspec.

Comment by Alex Miller [ 06/Aug/17 11:05 PM ]

As you can see, there is an existing patch here with substantial work on it already (and some early review from Rich and Stu). The most useful help on this at the moment would be feedback on the gaps/open questions in it.

Comment by Alex Miller [ 06/Aug/17 11:17 PM ]

Also, I should mention that I do not expect this to be finalized and included until we have finalized the spec forms themselves as they are still subject to change.

Comment by Alexander Kiel [ 27/Jun/18 7:34 AM ]

In the current patch, keywords referencing specs are missing except for keys in `s/keys`. What would the right way to model the keywords? One way would be to add `qualified-keyword?` as fourth option to the or-spec of `::spec`. Than `::spec` could be used as spec for the spec arg in `s/valid?`, `s/conform`, `s/form` and others.

Comment by Alex Miller [ 27/Jun/18 8:07 AM ]

Yeah, that would probably be good.

Comment by Alexander Kiel [ 27/Jun/18 10:45 AM ]

Another observation: Currently s/and and s/or are allowed to contain no preds:

(defspec clojure.spec.alpha/and (s/* ::spec))
(defspec clojure.spec.alpha/or (s/* (s/cat :tag keyword? :pred ::spec)))

there should be either a clear semantic definition what an empty form will mean or we should just use s/+ here and require at least one pred. For the and-form, we could define that (s/and) is the same as (s/and any?) and the same as just any?. For the or-form it is difficult, because s/conform returns tagged results. So conforming any value with (s/or) will result in ::s/invalid which renders an empty or-form useless.

Comment by Alex Miller [ 27/Jun/18 10:54 AM ]

There is a clear semantic definition for these - they are the same as clojure.core/and and clojure.core/or. As far as I'm concerned, this is all correct.

Comment by Alexander Kiel [ 28/Jun/18 7:33 AM ]

Thanks. I see. The documentation in clojure.core defines that (clojure.core/and) returns true and (clojure.core/or) returns nil.





[CLJ-2253] slurp doesn't properly handle URLs that contain authentication information Created: 19/Oct/17  Updated: 27/Jun/18  Resolved: 27/Jun/18

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

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

n/a


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

 Description   

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

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Doing nothing is always an option.

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

Where's the fun in that?

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

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

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

How about this approach (also tested as before):

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

?

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Comment by Erik Assum [ 27/Jun/18 3:54 PM ]

Uploaded 0003 patch which is basically 0002, but without the try catch, and with an added import

Comment by Alex Miller [ 27/Jun/18 4:46 PM ]

The more I think about this, the less I like it. slurp is (obviously) not intended to handle every possible remote resource retrieval and I see more downside than upside in moving in this direction. Declining...

Comment by Peter Monks [ 27/Jun/18 5:35 PM ]

Alex it would be good to better understand why you've chosen to draw this somewhat arbitrary line through HTTP retrieval support in slurp.

I don't think anyone is proposing that slurp should "handle every possible remote resource retrieval", but there are clear pragmatic advantages in properly supporting at least HTTP (given it's become the lingua franca of remote resource retrieval).

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

If you're not saying that it should handle every possible option then you are implicitly admitting there is a line. I'm drawing that line here. This is not a burning need or priority for anyone's production app. Use an http client lib if you need it.

Comment by Peter Monks [ 27/Jun/18 9:42 PM ]

Nice straw man you've constructed there Alex - nowhere did I say that there wasn't a line. Rather, and at the risk of sounding like a broken record, I am advocating for that line being around HTTP - all of it, not some arbitrary, Java-historical-oddity defined line.

And yes, this is a burning priority for several production apps I developed and operate. Sufficiently burning that I've hacked around this bug in those apps myself, something I would rather not have had to do. And as the patches above demonstrate, an HTTP client library is not needed for this - the core Java libraries can already handle this case, they just don't fully support the URI RFC when it comes to URIs provided as string values.

Comment by Alex Miller [ 27/Jun/18 10:02 PM ]

The point is, I'm not willing to sign up to support more than it does now and continue supporting it forever. You can handle this yourself by extending the protocol on the user side (which is the point of open extensions) or by using other libs, or by using Java directly.

Comment by Peter Monks [ 27/Jun/18 11:39 PM ]

We'll just have to agree to disagree that the maintenance cost of this ~16 line patch is greater than the benefit of slurp acting in a reasonable, spec-compliant way.





[CLJ-2333] Support java.nio.file.Path in clojure.java.io Created: 07/Mar/18  Updated: 27/Jun/18

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

Type: Enhancement Priority: Minor
Reporter: Michael Nygard Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: io

Attachments: Text File CLJ-2333-1.patch    
Approval: Triaged

 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.



 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.





[CLJ-2366] Bump spec lib deps Created: 27/Jun/18  Updated: 27/Jun/18  Resolved: 27/Jun/18

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

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

Attachments: Text File update-libs.patch    
Patch: Code
Approval: Ok

 Description   

Update spec.alpha and core.specs.alpha dep versions to latest (0.2.168 and 0.2.36 respectively).

Patch: update-libs.patch






[CLJ-2257] Documentation typo Created: 28/Oct/17  Updated: 27/Jun/18

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

Type: Defect Priority: Trivial
Reporter: Seb Martel Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring
Environment:

all


Attachments: Text File 0001-Documentation-typo.patch    
Patch: Code
Approval: Screened

 Description   

'methd' to 'method' in proxy documentation.

Screened: Alex Miller






[CLJ-1403] ns-resolve might throw ClassNotFoundException but should return nil Created: 14/Apr/14  Updated: 27/Jun/18

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler

Attachments: Text File 0001-CLJ-1403-ns-resolve-returns-nil-if-class-is-not-foun.patch     Text File clj-1403-2.patch    
Patch: Code and Test
Approval: Screened

 Description   

The doc of ns-resolve states that in case the symbol cannot be resolved, it should return nil.

user=> (doc ns-resolve)
-------------------------
clojure.core/ns-resolve
([ns sym] [ns env sym])
  Returns the var or Class to which a symbol will be resolved in the
  namespace (unless found in the environment), else nil.  Note that
  if the symbol is fully qualified, the var/Class to which it resolves
  need not be present in the namespace.
nil

However if the symbol contains dots and is not a resolvable Class, a ClassNotFoundException is thrown

user=> (ns-resolve *ns* 'foo)
nil  ;; expected
user=> (ns-resolve *ns* 'foo.bar)   ;; expect: nil
ClassNotFoundException foo.bar  java.net.URLClassLoader$1.run (URLClassLoader.java:372)
user=> (pst *e)
ClassNotFoundException foo.bar
	java.net.URLClassLoader$1.run (URLClassLoader.java:372)
	java.net.URLClassLoader$1.run (URLClassLoader.java:361)
	java.security.AccessController.doPrivileged (AccessController.java:-2)
	java.net.URLClassLoader.findClass (URLClassLoader.java:360)
	clojure.lang.DynamicClassLoader.findClass (DynamicClassLoader.java:61)
	java.lang.ClassLoader.loadClass (ClassLoader.java:424)
	java.lang.ClassLoader.loadClass (ClassLoader.java:357)
	java.lang.Class.forName0 (Class.java:-2)
	java.lang.Class.forName (Class.java:340)
	clojure.lang.RT.classForName (RT.java:2065)
	clojure.lang.Compiler.maybeResolveIn (Compiler.java:6963)
	clojure.core/ns-resolve (core.clj:4026)
nil

Approach: Catch CNFE when attempting to resolve a dotted symbol as a class and return nil instead.

Patch: clj-1403-2.patch

Screened by: Alex Miller
The attached patch makes ns-resolve return nil in that case instead of throwing an exception



 Comments   
Comment by Alex Miller [ 14/Apr/14 2:07 PM ]

Can you include the (pst *e) ?

Comment by Nicola Mometto [ 14/Apr/14 2:10 PM ]

Added result of (pst *e) in the description

Comment by Andy Fingerhut [ 02/Oct/14 11:36 AM ]

Nicola, the patch 0001-CLJ-1403-ns-resolve-returns-nil-if-class-is-not-foun.patch dated 31 Aug 2014 applies cleanly to latest Clojure master as of Oct 1 2014, but fails to compile with JDK8. I haven't checked whether it compiles cleanly with other JDK versions yet.

Comment by Nicola Mometto [ 02/Oct/14 11:48 AM ]

Updated the patch so that it compiles fine on JDK8

Comment by Alex Miller [ 22/Jan/16 3:03 PM ]

Updated patch to apply to master, no other changes, attribution retained.





[CLJ-1587] PersistentArrayMap's assoc doesn't respect HASHTABLE_THRESHOLD Created: 12/Nov/14  Updated: 27/Jun/18

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections, data-structures, maps

Attachments: Text File 0001-PersistentArrayMap-s-assoc-doesn-t-respect-HASHTABLE.patch    
Patch: Code
Approval: Screened

 Description   

Currently a map with more than 8 elements will be converted from a PersistentArrayMap to a PersistentHashMap, but if using assoc, it will take 9 elements before the conversion happens:

user=>  (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7})
clojure.lang.PersistentArrayMap
user=> (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7 8 8})
clojure.lang.PersistentHashMap
user=>  (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8))
clojure.lang.PersistentArrayMap
user=>  (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8 9 9))
clojure.lang.PersistentHashMap

After patch:

user=> (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7})
clojure.lang.PersistentArrayMap
user=> (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7 8 8})
clojure.lang.PersistentHashMap
user=> (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8))
clojure.lang.PersistentHashMap
user=> (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8 9 9))
clojure.lang.PersistentHashMap

Patch: 0001-PersistentArrayMap-s-assoc-doesn-t-respect-HASHTABLE.patch

Screened by: Alex Miller






[CLJ-1764] partition-by runs infinite loop when one element of infinite partition is accessed Created: 19/Jun/15  Updated: 27/Jun/18

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

Type: Defect Priority: Minor
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   
(def x (partition-by zero? (range)))
(first x)
;-> (0)
(first (second x))
;-> infinite loop

The reason is that partition-by counts and thus realizes each current partition to call itself recursively.

It seems like unexpected behavior, since the user may not intend to realize the entire partition.

Approach: Change from using seq to lazy-seq in its last line

Patch: clj-1764.patch

Screened-by: Alex Miller - I did a perf check too and seemed to be about the same, possibly even faster on average (gc effects mean there is a lot of deviation).

(def r (range 100000))
(dotimes [_ 20]
  (time (count (partition-by odd? r))))


 Comments   
Comment by Matthew Gilliard [ 23/Jul/15 2:36 PM ]

Patch as suggested by Leon, + test.





[CLJ-2349] When reporting uncaught ExceptionInfo from clojure.test, line number isn't calculated correctly Created: 30/Apr/18  Updated: 27/Jun/18

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

Type: Defect Priority: Minor
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: clojure.test

Attachments: Text File CLJ-2349-correctly-report-ex-info-line-in-clojure.patch    
Patch: Code
Approval: Screened

 Description   
(deftest t1
  (throw (ex-info "msg" {})))

(t/test-vars [#'t1])

ERROR in (t1) (core.clj:4593)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ExceptionInfo: msg
 at clojure.core$ex_info.invoke (core.clj:4593)
    datascript.test/fn (:3)
    clojure.test$test_var$fn__7670.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)
    clojure.test$test_vars$fn__7692$fn__7697.invoke (test.clj:722)
    clojure.test$default_fixture.invoke (test.clj:674)
...

Here core.clj:4593 is the line in clojure/core.clj where ex-info function is defined. Instead, test should report a place where ex-info is called

Clojure.test already does some filtering for other cases, so I just extended it with this particular (and quite common) case.

Screened by: Alex Miller






[CLJ-1832] unchecked-* functions have different behavior on primitive longs vs boxed Longs Created: 26/Oct/15  Updated: 27/Jun/18

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

Type: Defect Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: math

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

 Description   

The behavior of unchecked-* functions such as unchecked-add, unchecked-subtract, and unchecked-multiply give different results for primitive longs (expected) and boxed longs (can get overflow exceptions). For example:

user=> *clojure-version*
{:major 1, :minor 7, :incremental 0, :qualifier nil}
user=> (doc unchecked-multiply)
-------------------------
clojure.core/unchecked-multiply
([x y])
  Returns the product of x and y, both long.
  Note - uses a primitive operator subject to overflow.
nil
user=> (unchecked-multiply 2432902008176640000 21)
-4249290049419214848
user=> (unchecked-multiply 2432902008176640000 (Long. 21))

ArithmeticException integer overflow  clojure.lang.Numbers.throwIntOverflow (Numbers.java:1501)

Normally no one would use explicit boxed Long arguments like in the example above, but these can easily occur, unintentionally, if the arguments to the unchecked functions are not explicitly type hinted as primitive long values.

Approach: clj-1832.patch

Screened: Alex Miller



 Comments   
Comment by Alex Miller [ 26/Oct/15 9:03 AM ]

I think this is a reasonable complaint. The trickiness of handling is of course doing it without affecting performance for the non-error case.

Comment by Gary Fredericks [ 26/Oct/15 8:04 PM ]

My first thought was that there shouldn't be any perf concern because it's just a matter of modifying lines such as this one, where the type dispatching has already been done. But maybe you're thinking that that line has to be more complex since the arguments could be of various different numeric types, not just Long and Long?

Comment by Alex Miller [ 27/Oct/15 7:19 AM ]

That was a general comment. I haven't actually looked at the code changes necessary.

Comment by Alexander Kiel [ 11/Feb/16 4:38 AM ]

This costs me an hour today.

I'm with Gary as I see no performance issue. But I see a code amount issue, because the whole tree of add, multiply ... methods has to be repeated.

I would opt for a doc amendment which explains that the unchecked-* functions only work with primitive types. User which see a need for using unchecked math certainly have no problem doing a cast if necessary.

Comment by Gary Fredericks [ 11/Feb/16 11:33 AM ]

It's probably also worth mentioning that speed is not the only use case for unchecked operations – sometimes, e.g. with crypto algorithms, you actually want the weirder kind of arithmetic, and might not want to bother with primitives at first.

Comment by Alexander Kiel [ 12/Feb/16 2:38 PM ]

After a suggestion from Alex Miller, I started with the implementation route here: https://github.com/alexanderkiel/clojure/tree/clj-1832 But it's still work in progress.

Comment by Alexander Kiel [ 13/Feb/16 4:14 AM ]

This patch correctly implements all unchecked-* functions. It assumes that the issue exists only for longs because doubles are unchecked anyway and ratios have bigints.

Comment by Alex Miller [ 24/Feb/16 3:01 PM ]

This looks good to me but I do not see a Contributor Agreement on file for you Alexander. Can you sign one per here: http://clojure.org/community/contributing





[CLJ-2031] clojure.walk/postwalk does not preserve MapEntry type objects Created: 01/Oct/16  Updated: 27/Jun/18

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

Type: Defect Priority: Major
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Unresolved Votes: 13
Labels: walk

Attachments: File clj-2031-w-test.diff     File clj-2031-w-test-v2.diff    
Patch: Code and Test
Approval: Screened

 Description   

This came up on Slack. A naïve implementation of "lispify" to turn vectors into lists used this code:

(defn lispify [s]
  (w/postwalk (fn [e] (if (vector? e) (apply list e) e)) s))

But when called like this:

(lispify [:html {:a "b"} ""])

It produces this error: java.lang.ClassCastException: clojure.lang.Keyword cannot be cast to java.util.Map$Entry

My initial reaction was to change the condition to (and (vector? e) (not (map-entry? e))) but that still failed, because while walking the hash map, the MapEntry [:a "b"] was turned into a PersistentVector.

At this point, we can switch to using prewalk and it works as expected:

(defn lispify [s]
  (w/prewalk (fn [e] (if (and (vector? e) (not (map-entry? e))) (apply list e) e)) s))

Now we get the expected result:

boot.user> (lispify [:html {:a "b"} ""])
(:html {:a "b"} "")

This seems unintuitive at best and feels like a bug: postwalk should preserve the MapEntry type rather than converting it to a PersistentVector.

Cause: The problem seems to be this line https://github.com/clojure/clojure/blob/master/src/clj/clojure/walk.clj#L45:

(instance? clojure.lang.IMapEntry form) (outer (vec (map inner form)))

Proposed: Change to:

(instance? clojure.lang.IMapEntry form) (outer (clojure.lang.MapEntry/create (inner (first form)) (inner (second form)))))

This would preserve the type of the subelement.

Patch: clj-2031-w-test-v2.diff
Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 11/Oct/16 12:19 PM ]

seems reasonable

Comment by Jozef Wagner [ 27/Oct/16 8:19 AM ]

Added patch with test

Comment by Alex Miller [ 27/Oct/16 8:34 AM ]

Instead of the calls to .key and .val you should just call key and val.

Comment by Jozef Wagner [ 27/Oct/16 8:42 AM ]

Good catch, thanks! Added patch clj-2031-w-test-v2.diff that uses key and val instead.

Comment by Sean Corfield [ 27/Jul/17 3:21 PM ]

This keeps coming up on Slack – given there's a patch and this is prescreened, can it just get fixed as part of the next Clojure 1.9.0 Alpha/Beta build?

Comment by Alex Miller [ 27/Jul/17 3:39 PM ]

It's in a list for Rich to look at.





[CLJ-2231] Remove dep lib exclusions Created: 30/Aug/17  Updated: 27/Jun/18  Resolved: 27/Jun/18

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: build

Attachments: Text File remove-exclusions.patch    
Patch: Code

 Description   

Originally, the spec.alpha and core.specs.alpha lib deps pulled in an older version of Clojure itself as dependencies and they were excluded by Clojure.

These libs have been altered to rely on Clojure, etc as provided scope dependencies instead, so Clojure no longer needs to exclude them (as they are no longer transitive deps).

Note: before applying this patch, the pom must be updated to rely on a version of spec.alpha with these changes (but we haven't released one yet).

Patch: remove-exclusions.patch



 Comments   
Comment by Alex Miller [ 27/Jun/18 9:30 AM ]

I think this was done in a separate patch earlier.





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

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

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-1973] generate-proxy produces unpredictable method order in generated classes Created: 01/Jul/16  Updated: 27/Jun/18

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

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: Vetted

 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



 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-1952] include var->sym in clojure.core Created: 08/Jun/16  Updated: 27/Jun/18

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

Type: Feature Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File clj-1952.patch     Text File clj-1952-v2.patch    
Approval: Vetted

 Description   

A lot of libraries define their own variant of `var->sym`, clojure.spec recently did so aswell as a private var called `->sym`.

This ticket proposes to move it from `clojure.spec` to `clojure.core` as a public var named `var->sym` and to refactor the two variants to use it.

Patch: clj-1952-v2.patch
Prescreened by: Alex Miller



 Comments   
Comment by Jozef Wagner [ 01/Dec/16 1:38 PM ]

Added patch

Comment by Alex Miller [ 01/Dec/16 7:58 PM ]

Patch has whitespace warnings that should be removed.

Comment by Jozef Wagner [ 02/Dec/16 2:10 AM ]

Attached updated patch clj-1952-v2.patch that has no trailing whitespaces.





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

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

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

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

 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



 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-1905] loop should retain primitive int or float without widening Created: 29/Mar/16  Updated: 27/Jun/18

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

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    
Patch: Code and Test
Approval: Vetted

 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: 0001-CLJ-1905-remove-useless-widening-on-loop-bindings.patch

Prescreening comments: My main 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




[CLJ-1895] Remove loading of clojure.string in clojure.java.io Created: 22/Feb/16  Updated: 27/Jun/18

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

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

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

 Description   

clojure.core loads clojure.java.io to define slurp and spit. clojure.java.io loads clojure.string, solely for a single call to replace. This slows down Clojure core startup for no reason.

Approach: Replace clojure.string/replace call with a Java interop call to .replace. This saves about 18 ms during Clojure core startup.

Patch: clj-1895.patch






[CLJ-1891] New socket server startup proactively loads too much code, slowing boot time Created: 09/Feb/16  Updated: 27/Jun/18

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: server

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

 Description   

In the new socket server code, clojure.core.server is proactively loaded (regardless of whether servers are in the config), which will also load clojure.edn and clojure.string.

Approach: Delay loading of this code until the first server config is found. This improves startup time when not using the socket server about 0.05 s.

Patch: clj-1891.patch



 Comments   
Comment by Alexander Yakushev [ 02/Jan/18 9:17 AM ]

Bump. With the introduction of Spec, and considering the fact that clojure.core.server triggers the initialization of Spec, the benefit of solving this issue should now be bigger than 0.05 sec (more like 0.2 sec). See http://clojure-goes-fast.com/blog/clojures-slow-start .





[CLJ-1888] AReference#meta() is synchronized Created: 26/Jan/16  Updated: 27/Jun/18

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

Type: Enhancement Priority: Major
Reporter: Roger Kapsi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: PNG File aref-meta-after.png     PNG File aref-meta.png     Text File clj-1888-2.patch     Text File clj-1888.patch    
Patch: Code
Approval: Vetted

 Description   

We use Clojure for a "rules engine". Each function represents a rule and metadata describes the rule and provides some static configuration for the rule itself. The system is immutable and concurrent.

If two or more Threads invoke the same Var concurrently they end up blocking each other because AReference#meta() is synchronized (see attached screenshot, the red dots).

(defn 
  ^{:rule {:remote-address "127.0.0.1"}}
  example
  [request]
  (let [rule (:rule (meta #'example))]
    (= (:remote-address rule) (:remote-address request))))

Approach: Replace synchronized block with a rwlock for greater read concurrency. This approach removes meta read contention (see real world example in comments). However, it comes with the downsides of:

  • extra field for every AReference (all namespaces, vars, atoms, refs, and agents)
  • adds construction of lock into construction of AReference (affects perf and startup time)

Patch: clj-1888-2.patch replaces synchronized with a rwlock for greater read concurrency

Alternatives:

  • Use volatile for _meta and synchronized for alter/reset. Allow read of _meta just under the volatile - would this be safe enough?
  • Extend AReference from ReentrantReadWriteLock instead of holding one - this is pretty weird but would have a different (potentially better) footprint for memory/construction.


 Comments   
Comment by Alex Miller [ 26/Jan/16 10:19 PM ]

A volatile is not sufficient in alterMeta as you need to read/update/write atomically.

You could however use a ReadWriteLock instead of synchronized. I've attached a patch that does this - if you have a reproducible case I'd be interested to see how it affects what you see in the profiler.

There are potential issues that would need to be looked at - this will increase memory per reference (the lock instance) and slow down construction (lock construction) at the benefit of more concurrent reads.

Comment by Roger Kapsi [ 27/Jan/16 8:34 AM ]

Hey Alex,

I do have a reproducible case. The blocking has certainly disappeared after applying your patch (see attached picture). The remaining blocking code on these "WorkerThreads" is sun.nio.ch.SelectorImpl.select(long) (i.e. not clojure related).

You can repro it yourself by executing something like the code below concurrently in an infinite loop.

(defn 
  ^{:rule {:remote-address "127.0.0.1"}}
  example
  [request]
  (let [rule (:rule (meta #'example))]
    (= (:remote-address rule) (:remote-address request))))

Suggestions for the patch: Make the meta lock a final field and maybe pull the read/write locks into local variables to avoid the double methods calls.

alterMeta(...)
  Lock w = _metaLock.writeLock();
  w.lock();
  try {
    // ...
  } finally {
    w.unlock();
  }
}
Comment by Alex Miller [ 16/Mar/16 3:02 PM ]

Marking pre-screened,





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

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

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

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

 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

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



 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-1771] Support for multiple key(s)-value pairs in assoc-in Created: 29/Jun/15  Updated: 27/Jun/18

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

Type: Feature Priority: Major
Reporter: Griffin Smith Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: None
Environment:

All


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

 Description   

It would be nice if assoc-in supported multiple key(s)-to-value pairs (and threw an error when there were an even number of arguments, just like assoc):

user=> (assoc-in {} [:a :b] 1 [:c :d] 2)
{:a {:b 1}, :c {:d 2}}
user=> (assoc-in {} [:a :b] 1 [:c :d])
IllegalArgumentException assoc-in expects even number of arguments after map/vector, found odd number

Patch: clj-1771.patch

Prescreened by: Alex Miller



 Comments   
Comment by Matthew Gilliard [ 23/Jul/15 2:15 PM ]

Simple patch attached. I did not find any existing tests for assoc-in but I could add them if wanted.

Comment by Yehonathan Sharvit [ 19/Aug/16 10:19 AM ]

for the sake of symmetry with `assoc` I'd love to see this ticket fixed

Comment by Alex Miller [ 27/Nov/16 10:33 PM ]

Do you need the "if kvs" check?

Should have tests.

Comment by Matthew Gilliard [ 14/Jan/17 11:34 AM ]

Sorry for the delay - I don't get notifications from this JIRA for some reason.

The patch now includes tests.

Both `if` checks are necessary as we have 3 possible outcomes there:
1/ No more kvs (we are finished)
2/ More kvs (we need to recur)
3/ A sequence of keys but no value (throw IAE)





[CLJ-1986] Suppress printing namespace map literal syntax when only one namespaced key Created: 21/Jul/16  Updated: 27/Jun/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: maps, print

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

 Description   

Really an aesthetic choice, but right now maps with only a single namespaced key are printed in namespace map literal syntax:

user=> {:my.ns/b 1}
#:my.ns{:b 1}

And that seems unnecessarily complicated (and longer).

Proposal: Only print namespace map literal syntax when >1 key is using the same namespace.

Patch: clj-1986.patch






[CLJ-1095] Allow map-indexed to accept multiple collections (a la map) Created: 25/Oct/12  Updated: 27/Jun/18

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

Type: Feature Priority: Trivial
Reporter: Bo Jeanes Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File 0001-map-indexed-accepts-multiple-collections.patch     Text File 0002-Add-test-for-multi-collection-map-indexed-fn.patch