<< Back to previous view

[CLJ-1402] sort-by calls keyfn more times than is necessary Created: 11/Apr/14  Updated: 29/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Steve Kim Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: performance

Attachments: Text File CLJ-1402-v1.patch     Text File CLJ-1402-v2.patch    
Patch: Code
Approval: Triaged

 Description   

clojure.core/sort-by evaluates keyfn for every pairwise comparison. This is wasteful when keyfn is expensive to compute.

user=> (def keyfn-calls (atom 0))
#'user/keyfn-calls
user=> (defn keyfn [x] (do (swap! keyfn-calls inc) x))
#'user/keyfn
user=> @keyfn-calls
0
user=> (sort-by keyfn (repeatedly 10 rand))
(0.1647483850582695 0.2836687590331822 0.3222305842748623 0.3850390922996001 0.41965440953966326 0.4777580378736771 0.6051704988802923 0.659376178201709 0.8459820304223701 0.938863131161208)
user=> @keyfn-calls
44


 Comments   
Comment by Steve Kim [ 11/Apr/14 11:46 AM ]

CLJ-99 is a similar issue

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

Avoid using for before it's defined

Comment by Andy Fingerhut [ 25/May/15 5:13 PM ]

Michael, does your patch CLJ-1402-v2.patch intentionally modify the doc string of sort-by, because the sentence you are removing is now obsolete? If so, that would be good to mention explicitly in the comments here.

Comment by Michael Blume [ 26/May/15 2:41 PM ]

Yep, the patch changes sort-by so that it maps over the collection and then performs a sort on the resulting seq. This means arrays will be unmodified and a new seq created instead.

Comment by Alex Miller [ 29/Apr/16 10:10 AM ]

This patch seems like it could be slower, rather than faster, due to the extra allocation. Really needs perf tests before it can be seriously considered.





[CLJ-1911] min-key and max-key should return NaN if any of the argument is NaN Created: 08/Apr/16  Updated: 29/Apr/16

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

Type: Defect Priority: Minor
Reporter: Renzo Borgatti Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None
Environment:

Likely All. Including older version of Clojure.


Attachments: Text File CLJ-1911-contagious-NaN-and-tests.patch     Text File CLJ-1911-contagious-NaN.patch    
Patch: Code
Approval: Triaged

 Description   

It appears that min-key and max-key behave incorrectly (following Java that follows IEEE floating point convention):

(apply max-key last [[:a 10000] [:b (/ 0. 0)] [:c 0]])
[:c 0]

Not sure how this should then propagate forward, but definitely not silently. Options:

1. [:b NaN] (the first item to generate the NaN)
2. NaN (this is changing the expected type)
3. ArithmeticException Operation with at least one NaN operand.

If this was to be patched the same as it was for min/max (http://dev.clojure.org/jira/browse/CLJ-868) it will probably result in option 1.



 Comments   
Comment by Nicholas Antonov [ 14/Apr/16 9:36 PM ]

This implements the first solution of a contagious NaN in the same style as CLJ 868

Comment by Alex Miller [ 15/Apr/16 12:03 AM ]

Patch should have tests...

Comment by Nicholas Antonov [ 15/Apr/16 1:07 AM ]

This latest patch adds tests for min-key and max-key with and without NaN results, as there were none before.

Comment by Alex Miller [ 29/Apr/16 10:06 AM ]

This overlaps with CLJ-99, which has already been prescreened. I would like to base whatever changes this patch requires over the top of that ticket. To build this, apply the CLJ-99 patch, then branch, make you changes, and then create a patch relative to the clj-99 branch. Sorry that's a pain - usually patches don't collide at this level of conflict.





[CLJ-1910] Namespaced maps Created: 07/Apr/16  Updated: 28/Apr/16

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: print, reader

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

 Description   

A common usage of namespaced keywords and symbols is in providing attribute disambiguation in map contexts:

{:person/first "Han" :person/last "Solo" :person/ship 
  {:ship/name "Millenium Falcon" :ship/model "YT-1300f light freighter"}}

The namespaces provide value (disambiguation) but have the downside of being repetitive and verbose.

Namespaced maps are a reader (and printer) feature to specify a namespace context for a map.

  • Namespaced maps combine a default namespace with a map and yield a map.
  • Namespaced maps are reader macros starting with #: or #::, followed by a normal map definition.
    • #:sym indicates that sym is the default namespace for the map to follow.
    • #:: indicates that the default namespace auto-resolves to the current namespace.
    • #::sym indicates that sym should be auto-resolved using the current namespace's aliases OR any fully-qualified loaded namespace.
      • These rules match the rules for auto-resolved keywords.
  • A namespaced map is read with the following differences from normal maps:
    • A keyword or symbol key without a namespace is read with the default namespace as its namespace.
    • Keys that are not symbols or keywords are not affected.
    • Keys that specify an explicit namespace are not affected EXCEPT the special namespace _, which is read with NO namespace. This allows the specification of bare keys in a namespaced map.
    • Values are not affected.
    • Nested map keys are not affected.
  • The edn reader supports #: but not #:: with the same rules as above.
  • Maps will be printed in namespaced map form only when:
    • All map keys are keywords or symbols
    • All map keys are namespaced
    • All map keys have the same namespace

Examples:

;; same as above - notice you can nest #: maps and this is a case where the printer roundtrips
user=> #:person{:first "Han" :last "Solo" :ship #:ship{:name "Millenium Falcon" :model "YT-1300f light freighter"}}
#:person{:first "Han" :last "Solo" :ship #:ship{:name "Millenium Falcon" :model "YT-1300f light freighter"}}

;; effects on keywords with ns, without ns, with _ ns, and non-kw
user=> #:foo{:kw 1, :n/kw 2, :_/bare 3, 0 4}
{:foo/kw 1, :n/kw 2, :bare 3, 0 4}

;; auto-resolved namespaces (will use user as the ns)
user=> #::{:kw 1, :n/kw 2, :_/bare 3, 0 4}
:user/kw 1, :n/kw 2, :bare 3, 0 4}

;; auto-resolve alias s to clojure.string
user=> (require '[clojure.string :as s])
nil
user=> #::s{:kw 1, :n/kw 2, :_/bare 3, 0 4}
{:clojure.string/kw 1, :n/kw 2, :bare 3, 0 4}

;; to show symbol changes, we'll quote the whole thing to avoid evaluation
user=> '#::{a 1, n/b 2, _/c 3}
{user/a 1, n/b 2, c 3}

;; edn reader also supports (only) the #: syntax
user=> (clojure.edn/read-string "#:person{:first \"Han\" :last \"Solo\" :ship #:ship{:name \"Millenium Falcon\" :model \"YT-1300f light freighter\"}}")
#:person{:first "Han", :last "Solo", :ship #:ship{:name "Millenium Falcon", :model "YT-1300f light freighter"}}

Patch: clj-1910-2.patch

Screener notes:

  • Autoresolution supports fully-qualified loaded namespaces (like auto-resolved keywords)
  • TODO: pprint support for namespaced maps
  • TODO: printer flag to suppress printing namespaced maps


 Comments   
Comment by Nicola Mometto [ 08/Apr/16 3:57 AM ]

1- yes please. that's consistent with how tagged literals work.
2- no please. that would make the proposed syntax useless for e.g. Datomic schemas, for which I think this would be a good fit to reduce noise
3- yes please
4- yes please, consistency over print methods is important

Comment by Nicola Mometto [ 08/Apr/16 4:00 AM ]

Quoting from a post I wrote on the clojure-dev ML:

  • I really don't like the idea of special-casing `_` here, users are already confused about idioms like `[.. & _]` thinking that `_` is some special token/variable. Making it an actual special token in some occasion wouldn't help.
  • I also don't like how we're making the single `:` auto-qualify keywords when used within the context of a qualifying-map. Auto-qualifying keywords has always been the job of the double `::`, changing this would introduce (IMO) needless cognitive overhead.
  • The current impl treats `#:foo{'bar 1}` and `'#:foo{bar 1}` differently. I can see why is that, but the difference might be highly unintuitive to some.
  • The current proposal makes it feel like quote is auto-qualifying symbols , when that has always been the job of syntax-quote. I know that's not correct, but that's how it's perceived.

Here's an alternative syntax proposal that handles all those issues:

  • No #::, only #:foo or #::foo
  • No auto-resolution of symbols when the namespaced-map is quoted, only when syntax-quoted
  • No special-casing of `_`
  • No auto-resolution of single-colon keywords

Here's how the examples in the ticket description would look:

#:person{::first "Han", ::last "Solo", ::ship #:ship{::name "Millenium Falcon", ::model "YT-1300f light freighter"}}
;=> {:person/first "Han" :person/last "Solo" :person/ship {:ship/name "Millenium Falcon" :ship/model "YT-1300f light freighter"}}

#:foo{::kw 1, :n/kw 2, :bare 3, 0 4}
;=> {:foo/kw 1, :n/kw 2, :bare 3, 0 4}

{::kw 1, :n/kw 2, bare 3, 0 4}
;=> {:user/kw 1, :n/kw 2, :bare 3, 0 4}

Note in the previous example how we don't need `#::` at all – `::` already does that job for us

(require '[clojure.string :as s])
#::s{::kw 1, :n/kw 2, bare 3, 0 4}
;=> {:clojure.string/kw 1, :n/kw 2, :bare 3, 0 4}

`{a 1, n/b 2, ~'c 3}
;=> {user/a 1, n/b 2, c 3}

Again, no need for `#::` here, we can just rely on the existing auto-qualifying behaviour of `.

`#:foo{a 1, n/b 2}
;=> {foo/a 1, n/b 2}

I think this would be more consistent with the existing behaviour – it's basically just making `#:foo` or `#::foo` mean: in the top-level keys of the following map expression, resolve keywords/symbols as if ns was bound to `foo`, rather than introducing new resolution rules and special tokens.

I realize that this proposal wouldn't work with EDNReader as-is, given its lack of support for `::` and "`". I don't have a solution to that other than "let's just bite the bullet and implement them there too", but maybe that's not acceptable.

Comment by Alex Miller [ 08/Apr/16 8:45 AM ]

Nicola, thanks for the proposal, we talked through it. We share your dislike for :_/kw syntax and you should consider that a placeholder for this behavior for the moment - it may be removed or replaced before we get to a published release.

For the rest of it:

  • requiring syntax quote is a non-starter
  • supporting a mixture of default ns and the current ns is important and this is not possible with your proposal. Like #:foo{:bar 1 ::baz 2}.
  • there is a lot of value to changing the scope of a map without modifying the contents, which is an advantage of the syntax in the ticket
Comment by Christophe Grand [ 08/Apr/16 10:31 AM ]

Why restrict this feature to a single namespace? (this doesn't preclude a shorthand for the single mapping) I'd like to locally define aliases (and default ns).

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

We already have namespace level aliases. You can use :: in the map to leverage those aliases (independently from the default ns):

(ns app 
  (:require [my.domain :as d]
            [your.domain :as y]))

#::{:svc 1, ::d/name 2, ::y/name 3}

;;=> {:app/svc 1, :my.domain/name 2, :your.domain/y 3}
Comment by Christophe Grand [ 11/Apr/16 4:03 AM ]

Alex, if existing namespace level aliases are enough when there's more than one namespace used in the key set I fail to understand the real value of this proposal.

Okay I'm lying a little: there are no aliases in edn, so this would bring aliases to edn (and allows printers to factor/alias namespaces out). And for Clojure code you can't define an alias to a non-existing namespace – and I believe that this implementation wouldn't check namespace existence when resolving the default ns #:person{:name}.

Still my points hold for edn (and that's where the value of this proposal seems to be): why not allows local aliases too?

#:person #:employee/e {:name "John Smith", :e/eid "012345"}
;=> {:person/name "John Smith", :employee/eid "012345"}

I have another couple of questions:

  • should it apply to other datatypes?
  • should it be transitive?
Comment by Alex Miller [ 28/Apr/16 1:33 PM ]

New patch rev supports spaces between the namespace part #:foo and the map in both LispReader and EdnReader.





[CLJ-1919] Destructuring support for namespaced keys and syms Created: 27/Apr/16  Updated: 27/Apr/16

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: destructuring

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

 Description   

Expand destructuring to better support a set of keys (or syms) from a map when the keys share the same namespace.

Example:

(def m {:person/first "Darth" :person/last "Vader" :person/email "darth@death.star"})

(let [{:keys [person/first person/last person/email]} m]
  (format "%s %s - %s" first last email))

Proposed: The special :keys and :syms keywords used in associative destructuring may now have a namespace (eg :person/keys). That namespace will be applied during lookup to all listed keys or syms when they are retrieved from the input map.

Example (also uses the new literal syntax for namespaced maps from CLJ-1910):

(def m #:person{:first "Darth" :last "Vader" :email "darth@death.star"})

(let [{:person/keys [first last email]} m]
  (format "%s %s - %s" first last email))
  • The key list after :ns/keys should contain either non-namespaced symbols or non-namespaced keywords. Symbols are preferred.
  • The key list after :ns/syms should contain non-namespaced symbols.
  • As :ns/keys and :ns/syms are read as normal keywords, auto-resolved keywords work as well: ::keys, ::alias/keys, etc.
  • Clarification - the :or defaults map always uses non-namespaced symbols as keys - that is, they are always the same as the locals being created (not the keys being looked up in the map). No change in behavior here, just trying to be explicit - this was not previously well-documented for namespaced key lookup and was broken. The attached patch fixes this behavior.

Patch: clj-1919.patch






[CLJ-1647] infinite loop in 'partition' and 'partition-all' when 'step' or 'n' is not positive Created: 20/Jan/15  Updated: 27/Apr/16

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

Type: Defect Priority: Minor
Reporter: Kevin Woram Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: checkargs, newbie

Attachments: Text File clj-1647_2.patch     Text File clj-1647_3.patch     Text File clj-1647.patch     Text File kworam-clj-1647.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

If you pass a non-positive value of 'n' or 'step' to partition, you get an infinite loop. Here are a few examples:

(partition 0 [1 2 3])
(partition 1 -1 [1 2 3])

Cause: partition and partition-all do not check that n and step are positive.

Approach: Add checks to partition and partition-all that n and step are positive.

Patch: clj-1647_3.patch

Prescreened by: Alex Miller



 Comments   
Comment by Alex Miller [ 03/Feb/15 5:34 PM ]

Also see CLJ-764

Comment by Alex Miller [ 29/Apr/15 12:02 PM ]

Needs a perf check when done.

Comment by Kevin Woram [ 16/May/15 1:58 PM ]

patch file to fix clj-1647

Comment by Kevin Woram [ 16/May/15 2:19 PM ]

Since 'n' and 'step' remain unchanged from the initial function call through all of the recursive self-calls, I only need to verify that they are positive once, on the initial call.

I therefore created functions 'internal-partition' and 'internal-partition-all' whose implementations are identical to the current versions of 'partition' and 'partition-all'.

I then added preconditions that 'step' and 'n' must be positive to the 'partition' and 'partition-all' functions, and made them call 'internal-partition' and 'internal-partition-all' respectively to do the work.

Comment by Alex Miller [ 17/May/15 8:14 AM ]

There are a lot of unrelated whitespace changes in this patch - can you supply a smaller patch with only the change at issue? Also needs tests.

Comment by Kevin Woram [ 17/May/15 2:05 PM ]

I will supply a patch file without the whitespace changes.

I know there are some existing functionality tests for 'partition' and 'partition-all' in test_clojure\sequences.clj and test_clojure\transducers.clj. I don't think I need to add more functionality tests, but I think I should add:

1. Tests that verify that non-positive 'step' and 'n' parameters are rejected.
2. Tests that show that 'partition' and 'partition-all' performance has not degraded significantly.

Could you give me some guidance on how to develop and add these tests?

Comment by Alex Miller [ 17/May/15 3:31 PM ]

You should add #1 to the patch. For #2, you can just do the timings before/after (criterium is a good tool for this) and put the results in the description.

Comment by Kevin Woram [ 22/May/15 4:04 PM ]

I have coded up the tests for #1 and taken some 'before' timings for #2 using criterium.

I have been stumped by a problem for hours now and I need to get some help. I made my changes to 'partition' and 'partition-all' in core.clj and then did 'mvn package' to build the jar. I executed 'target>java -cp clojure-1.7.0-master-SNAPSHOT.jar clojure.main' to test out my patched version of clojure interactively. The (source ...) function shows that my source changes for both 'partition' and 'partition-all' are in place. My change to 'partition-all' seems to be working but my change to 'partition' is not. As far as I can tell, they should both throw an AssertionError with the input parameters I am providing.

Any help would be greatly appreciated.

user=> (source partition)
(defn partition
"Returns a lazy sequence of lists of n items each, at offsets step
apart. If step is not supplied, defaults to n, i.e. the partitions
do not overlap. If a pad collection is supplied, use its elements as
necessary to complete last partition upto n items. In case there are
not enough padding elements, return a partition with less than n items."
{:added "1.0"
:static true}
([n coll]
{:pre [(pos? n)]}
(partition n n coll))
([n step coll]
{:pre [(pos? n) (pos? step)]}
(internal-partition n step coll))
([n step pad coll]
{:pre [(pos? n) (pos? step)]}
(internal-partition n step pad coll)))
nil
user=> (partition -1 [1 2 3])
()
user=> (source partition-all)
(defn partition-all
"Returns a lazy sequence of lists like partition, but may include
partitions with fewer than n items at the end. Returns a stateful
transducer when no collection is provided."
{:added "1.2"
:static true}
([^long n]
(internal-partition-all n))
([n coll]
(partition-all n n coll))
([n step coll]
{:pre [(pos? n) (pos? step)]}
(internal-partition-all n step coll)))
nil
user=> (partition-all -1 [1 2 3])
AssertionError Assert failed: (pos? n) clojure.core/partition-all (core.clj:6993)

Comment by Alex Miller [ 22/May/15 4:47 PM ]

Did you mvn clean? Or rm target?

Comment by Kevin Woram [ 24/May/15 11:46 PM ]

Yes, I did mvn clean and verified that clojure-1.7.0-master-SNAPSHOT.jar had the expected date-time stamp before doing the interactive test. I even went so far as to retrace my steps on my Macbook on the theory that maybe there was a Windows-specific build problem.

My change to partition-all works as expected but my change to partition does not. However, if I copy the result of the call to (source partition) and execute it (replacing clojure.core/partition with user/partition), user/partition works as expected. I don't understand why my change to clojure.core/partition isn't taking effect.

Comment by Andy Fingerhut [ 25/May/15 1:27 AM ]

Kevin, I do not know the history of your Clojure source tree, but if you ever ran 'ant' in it, that creates jar files in the root directory, whereas 'mvn package' creates them in the target directory. It wasn't clear from your longer comment above whether the 'java -cp ...' command you ran pointed at the one in the target directory. That may not be the cause of the issue you are seeing, but I don't yet have any guesses what else it could be.

Comment by Alex Miller [ 13/Jul/15 1:17 PM ]

What's the status of this?

Comment by Kevin Woram [ 16/Jul/15 10:20 PM ]

Alex, I moved to Seattle and took a permanent position with Microsoft recently. This has kept me very busy and I haven't been able to spend time on Clojure at all. I probably won't be able to devote time to Clojure for another month or two.

Comment by Alex Miller [ 16/Jul/15 10:46 PM ]

Thanks for the heads up.

Comment by Matthew Gilliard [ 23/Jul/15 2:51 PM ]

Kevin, Alex, I could pick this up if you like?

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:41 PM ]

Sorry, browser fail

Comment by Kevin Woram [ 25/Jul/15 7:03 PM ]

Thanks for picking it up Matthew, I appreciate it!

Comment by Kevin Woram [ 25/Jul/15 7:03 PM ]

Thanks for picking it up Matthew, I appreciate it!

Comment by Matthew Gilliard [ 27/Jul/15 11:30 AM ]

New patch: clj-1647.patch

Includes tests, fewer whitespace changes, manually thrown IAEs. I'll do some basic benchmarking soon, although I expect the overhead to be quite low as we're only checking the arguments once.

Kevin, the reason your patch was working for partition-all but not partition is that partition is defined early-ish in the bootstrapping process and the {:pre .. :post ..} maps aren't read by defn until it's enhanced later on.

Comment by Kevin Woram [ 27/Jul/15 12:10 PM ]

Thanks for solving that mystery Matthew!

Comment by Alex Miller [ 10/Mar/16 2:45 PM ]

Patch looks basically good. Minor changes:

  • internal-partition and internal-partition-all should be marked private with defn-.
  • Commit description should start with "CLJ-1647"
Comment by Matthew Gilliard [ 26/Apr/16 10:32 AM ]

I added clj-1647_2.patch to supersede other patches on this issue. Jira ref is added to commit msg and defn- used where possible (defn- is not defined until after one of the private fns but there is the ^:private metadata added manually)

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

The patch changes add-annotation from defn- to defn but that seems unrelated to the intent of the patch?

Comment by Matthew Gilliard [ 27/Apr/16 3:15 AM ]

Thanks for looking so quickly Alex - sorry about that error in add-annotation. See clj-1647_3.patch





[CLJ-1768] quote of an empty lazyseq produces an error when evaled Created: 24/Jun/15  Updated: 26/Apr/16

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

Type: Defect Priority: Minor
Reporter: Tim Engler Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Triaged

 Description   
user=> (eval `'())
()
user=> `'~(map identity ())
(quote ())
user=> (eval `'~(map identity ()))    ;; expected: ()
CompilerException java.lang.UnsupportedOperationException: Unknown Collection type, compiling:(NO_SOURCE_PATH:5:1)
user=> (prn *e)
#error {
 :cause "Unknown Collection type"
 :via
 [{:type clojure.lang.Compiler$CompilerException
   :message "java.lang.UnsupportedOperationException: Unknown Collection type, compiling:(NO_SOURCE_PATH:5:1)"
   :at [clojure.lang.Compiler analyzeSeq "Compiler.java" 6730]}
  {:type java.lang.UnsupportedOperationException
   :message "Unknown Collection type"
   :at [clojure.lang.Compiler$EmptyExpr emit "Compiler.java" 2929]}]
 :trace
 [[clojure.lang.Compiler$EmptyExpr emit "Compiler.java" 2929]
  [clojure.lang.Compiler$BodyExpr emit "Compiler.java" 5905]
  [clojure.lang.Compiler$FnMethod doEmit "Compiler.java" 5453]
  [clojure.lang.Compiler$FnMethod emit "Compiler.java" 5311]
  [clojure.lang.Compiler$FnExpr emitMethods "Compiler.java" 3843]
  [clojure.lang.Compiler$ObjExpr compile "Compiler.java" 4489]
  [clojure.lang.Compiler$FnExpr parse "Compiler.java" 3983]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6721]
  [clojure.lang.Compiler analyze "Compiler.java" 6524]
  [clojure.lang.Compiler eval "Compiler.java" 6779]
  [clojure.lang.Compiler eval "Compiler.java" 6745]
  [clojure.core$eval invoke "core.clj" 3081]
  ;; elided rest
nil
user=> (eval `'~(map identity '(x)))
(x)

Cause: In the empty list case, the compiler here sees a LazySeq. I suspect something earlier in the stack should be producing an empty list instead, but haven't tracked it back yet.



 Comments   
Comment by Tim Engler [ 26/Apr/16 4:17 AM ]

Still exists in clojure 1.8





[CLJ-1918] Document await that it will never return if shutdown-agents was called Created: 25/Apr/16  Updated: 25/Apr/16

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

Type: Enhancement Priority: Trivial
Reporter: Ruslan Al-Fakikh Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: agents, docstring

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

 Description   

Undocumented behavior or the "await" function: it will never return if shutdown-agents was called.

This was a surprise to me to find yet another condition when await never returns.

(def my-agent (agent 0)) 

(defn sleep-and-inc [number] 
  (Thread/sleep 3000) 
  (println "action number" number "complete") 
  (inc number)) 

(println "sending off 2 times") 
(send-off my-agent sleep-and-inc) 
(send-off my-agent sleep-and-inc) 
(println "sending off complete") 

;making sure all the actions have completed to make it simple, 
;otherwise only the first action will be executed 
(Thread/sleep 7000) 

(shutdown-agents) 

(println "starting await") 
(await my-agent) 
(println "await complete");this will never happen 

;here is how it behaves: 
;sending off 2 times 
;sending off complete 
;action number 0 complete 
;action number 1 complete 
;starting await 
;...hanging forever...

Proposal: Change the docstring for clojure.core/await from "...Will never return if a failed agent is restarted with :clear-actions true." to
"...Will never return if a failed agent is restarted with :clear-actions true or shutdown-agents was called."

Patch: CLJ-1918.patch

Prescreened by: Alex Miller






[CLJ-1917] internal-reduce extended on StringSeq calls `.length` on every iteration step Created: 24/Apr/16  Updated: 25/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Dimitrios Piliouras Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance
Environment:

n/a


Approval: Triaged

 Description   

internal-reduce extended on StringSeq calls `.length` (on the same String object) upon every iteration step [1]. There is absolutely no need for this as the length of a String cannot change. Therefore, it can be bound once (in the `let` a couple of lines up) and used thereafter.

[1]: https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/protocols.clj#L151






[CLJ-1901] amap calls `alength` at every iteration step Created: 13/Mar/16  Updated: 24/Apr/16

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

Type: Enhancement Priority: Major
Reporter: Dimitrios Piliouras Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: arrays, performance
Environment:

JVM


Attachments: Text File fix_amap.patch    
Patch: Code
Approval: Prescreened

 Description   

During the 1.7 => 1.8 upgrade `areduce` was fixed to not call `alength` on the same thing, at every single iteration step. However, `amap` which suffers from the same issue was not fixed (even though exactly the same fix applies).

Example:

(def an-array (long-array 100000 0))
(dotimes [_ 50]
  (time (amap ^longs an-array idx ret (+ 1 (aget ^longs an-array idx)))))

Before (last time): 0.3930 ms
After (last time): 0.3459 ms

Patch: fix_amap.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 13/Mar/16 4:39 PM ]

Thanks!

Comment by Dimitrios Piliouras [ 24/Apr/16 1:33 PM ]

Not a problem. I actually noticed a very similar thing in the `internal-reduce` implementation for StringSeq [1]. The `.length()` method is called on the same String on every single iteration step, even though it is a constant. Is that easy enough to be sorted without me submitting another trivial patch? Thanks in advance...

https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/protocols.clj#L151

Comment by Alex Miller [ 24/Apr/16 1:48 PM ]

Separate ticket would be preferred, thanks.

Comment by Dimitrios Piliouras [ 24/Apr/16 2:32 PM ]

Sure thing, I'll create it now.





[CLJ-1322] doseq with several bindings causes "ClassFormatError: Invalid Method Code length" Created: 10/Jan/14  Updated: 24/Apr/16

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

Type: Defect Priority: Major
Reporter: Miikka Koskinen Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: None
Environment:

Clojure 1.5.1, java 1.7.0_25, OpenJDK Runtime Environment (IcedTea 2.3.10) (7u25-2.3.10-1ubuntu0.12.04.2)


Attachments: Text File doseq-bench.txt     Text File doseq.patch     File script.clj    
Patch: Code
Approval: Incomplete

 Description   

Important Perf Note the new impl is faster for collections that are custom-reducible but not chunked, and is also faster for large numbers of bindings. The original implementation is hand tuned for chunked collections, and wins for larger chunked coll/smaller binding count scenarios, presumably due to the fn call/return tracking overhead of reduce. Details are in the comments.
Screened By
Patch doseq.patch

user=> (def a1 (range 10))
#'user/a1
user=> (doseq [x1 a1 x2 a1 x3 a1 x4 a1 x5 a1 x6 a1 x7 a1 x8 a1] (do))
CompilerException java.lang.ClassFormatError: Invalid method Code length 69883 in class file user$eval1032, compiling:(NO_SOURCE_PATH:2:1)

While this example is silly, it's a problem we've hit a couple of times. It's pretty surprising when you have just a couple of lines of code and suddenly you get the code length error.



 Comments   
Comment by Kevin Downey [ 18/Apr/14 12:20 AM ]

reproduces with jdk 1.8.0 and clojure 1.6

Comment by Nicola Mometto [ 22/Apr/14 5:35 PM ]

A potential fix for this is to make doseq generate intermediate fns like `for` does instead of expanding all the code directly.

Comment by Ghadi Shayban [ 25/Jun/14 8:39 PM ]

Existing doseq handles chunked-traversal internally, deciding the
mechanics of traversal for a seq. In addition to possibly conflating
concerns, this is causing a code explosion blowup when more bindings are
added, approx 240 bytes of bytecode per binding (without modifiers).

This approach redefs doseq later in core.clj, after protocol-based
reduce (and other modern conveniences like destructuring.)

It supports the existing :let, :while, and :when modifiers.

New is a stronger assertion that modifiers cannot come before binding
expressions. (Same semantics as let, i.e. left to right)

valid: [x coll :when (foo x)]
invalid: [:when (foo x) x coll]

This implementation does not suffer from the code explosion problem.
About 25 bytes of bytecode + 1 fn per binding.

Implementing this without destructuring was not a party, luckily reduce
is defined later in core.

Comment by Andy Fingerhut [ 26/Jun/14 12:25 AM ]

For anyone reviewing this patch, note that there are already many tests for correct functionality of doseq in file test/clojure/test_clojure/for.clj. It may not be immediately obvious, but every test for 'for' defined with deftest-both is a test for 'for' and also for 'doseq'.

Regarding the current implementation of doseq: it in't simply that it is too many bytes per binding, it is that the code size doubles with each additional binding. See these results, which measures size of the macroexpanded form rather than byte code size, but those two things should be fairly linearly related to each other here:

(defn formsize [form]
  (count (with-out-str (print (macroexpand form)))))

user=> (formsize '(doseq [x (range 10)] (print x)))
652
user=> (formsize '(doseq [x (range 10) y (range 10)] (print x y)))
1960
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10)] (print x y z)))
4584
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10)] (print x y z w)))
9947
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10) p (range 10)] (print x y z w p)))
20997

Here are results for the same expressions after Ghadi's patch doseq.patch dated June 25 2014:

user=> (formsize '(doseq [x (range 10)] (print x)))
93
user=> (formsize '(doseq [x (range 10) y (range 10)] (print x y)))
170
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10)] (print x y z)))
247
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10)] (print x y z w)))
324
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10) p (range 10)] (print x y z w p)))
401

It would be good to see some performance results with and without this patch, too.

Comment by Stuart Halloway [ 28/Jun/14 2:21 PM ]

In the tests below, the new impl is called "doseq2", vs. the original impl "doseq"

(def hund (into [] (range 100)))
(def ten (into [] (range 10)))
(def arr (int-array 100))
(def s "superduper")

;; big seq, few bindings: doseq2 LOSES
(dotimes [_ 5]
  (time (doseq [a (range 100000000)])))
;; 1.2 sec

(dotimes [_ 5]
  (time (doseq2 [a (range 100000000)])))
;; 1.8 sec

;; small unchunked reducible, few bindings: doseq2 wins
(dotimes [_ 5]
  (time (doseq [a s b s c s])))
;; 0.5 sec

(dotimes [_ 5]
  (time (doseq2 [a s b s c s])))
;; 0.2 sec

(dotimes [_ 5]
  (time (doseq [a arr b arr c arr])))
;; 40 msec

(dotimes [_ 5]
  (time (doseq2 [a arr b arr c arr])))
;; 8 msec

;; small chunked reducible, few bindings: doseq2 LOSES
(dotimes [_ 5]
  (time (doseq [a hund b hund c hund])))
;; 2 msec

(dotimes [_ 5]
  (time (doseq2 [a hund b hund c hund])))
;; 8 msec

;; more bindings: doseq2 wins bigger and bigger
(dotimes [_ 5]
  (time (doseq [a ten b ten c ten d ten ])))
;; 2 msec

(dotimes [_ 5]
  (time (doseq2 [a ten b ten c ten d ten ])))
;; 0.4 msec

(dotimes [_ 5]
  (time (doseq [a ten b ten c ten d ten e ten])))
;; 18 msec

(dotimes [_ 5]
  (time (doseq2 [a ten b ten c ten d ten e ten])))
;; 1 msec
Comment by Ghadi Shayban [ 28/Jun/14 6:23 PM ]

Hmm, I cannot reproduce your results.

I'm not sure whether you are testing with lein, on what platform, what jvm opts.

Can we test using this little harness instead directly against clojure.jar? I've attached a the harness and two runs of results (one w/ default heap, the other 3GB w/ G1GC)

I added a medium and small (range) too.

Anecdotally, I see doseq2 outperform in all cases except the small range. Using criterium shows a wider performance gap favoring doseq2.

I pasted the results side by side for easier viewing.

core/doseq                          doseq2
"Elapsed time: 1610.865146 msecs"   "Elapsed time: 2315.427573 msecs"
"Elapsed time: 2561.079069 msecs"   "Elapsed time: 2232.479584 msecs"
"Elapsed time: 2446.674237 msecs"   "Elapsed time: 2234.556301 msecs"
"Elapsed time: 2443.129809 msecs"   "Elapsed time: 2224.302855 msecs"
"Elapsed time: 2456.406103 msecs"   "Elapsed time: 2210.383112 msecs"

;; med range, few bindings:
core/doseq                          doseq2
"Elapsed time: 28.383197 msecs"     "Elapsed time: 31.676448 msecs"
"Elapsed time: 13.908323 msecs"     "Elapsed time: 11.136818 msecs"
"Elapsed time: 18.956345 msecs"     "Elapsed time: 11.137122 msecs"
"Elapsed time: 12.367901 msecs"     "Elapsed time: 11.049121 msecs"
"Elapsed time: 13.449006 msecs"     "Elapsed time: 11.141385 msecs"

;; small range, few bindings:
core/doseq                          doseq2
"Elapsed time: 0.386334 msecs"      "Elapsed time: 0.372388 msecs"
"Elapsed time: 0.10521 msecs"       "Elapsed time: 0.203328 msecs"
"Elapsed time: 0.083378 msecs"      "Elapsed time: 0.179116 msecs"
"Elapsed time: 0.097281 msecs"      "Elapsed time: 0.150563 msecs"
"Elapsed time: 0.095649 msecs"      "Elapsed time: 0.167609 msecs"

;; small unchunked reducible, few bindings:
core/doseq                          doseq2
"Elapsed time: 2.351466 msecs"      "Elapsed time: 2.749858 msecs"
"Elapsed time: 0.755616 msecs"      "Elapsed time: 0.80578 msecs"
"Elapsed time: 0.664072 msecs"      "Elapsed time: 0.661074 msecs"
"Elapsed time: 0.549186 msecs"      "Elapsed time: 0.712239 msecs"
"Elapsed time: 0.551442 msecs"      "Elapsed time: 0.518207 msecs"

core/doseq                          doseq2
"Elapsed time: 95.237101 msecs"     "Elapsed time: 55.3067 msecs"
"Elapsed time: 41.030972 msecs"     "Elapsed time: 30.817747 msecs"
"Elapsed time: 42.107288 msecs"     "Elapsed time: 19.535747 msecs"
"Elapsed time: 41.088291 msecs"     "Elapsed time: 4.099174 msecs"
"Elapsed time: 41.03616 msecs"      "Elapsed time: 4.084832 msecs"

;; small chunked reducible, few bindings:
core/doseq                          doseq2
"Elapsed time: 31.793603 msecs"     "Elapsed time: 40.082492 msecs"
"Elapsed time: 17.302798 msecs"     "Elapsed time: 28.286991 msecs"
"Elapsed time: 17.212189 msecs"     "Elapsed time: 14.897374 msecs"
"Elapsed time: 17.266534 msecs"     "Elapsed time: 10.248547 msecs"
"Elapsed time: 17.227381 msecs"     "Elapsed time: 10.022326 msecs"

;; more bindings:
core/doseq                          doseq2
"Elapsed time: 4.418727 msecs"      "Elapsed time: 2.685198 msecs"
"Elapsed time: 2.421063 msecs"      "Elapsed time: 2.384134 msecs"
"Elapsed time: 2.210393 msecs"      "Elapsed time: 2.341696 msecs"
"Elapsed time: 2.450744 msecs"      "Elapsed time: 2.339638 msecs"
"Elapsed time: 2.223919 msecs"      "Elapsed time: 2.372942 msecs"

core/doseq                          doseq2
"Elapsed time: 28.869393 msecs"     "Elapsed time: 2.997713 msecs"
"Elapsed time: 22.414038 msecs"     "Elapsed time: 1.807955 msecs"
"Elapsed time: 21.913959 msecs"     "Elapsed time: 1.870567 msecs"
"Elapsed time: 22.357315 msecs"     "Elapsed time: 1.904163 msecs"
"Elapsed time: 21.138915 msecs"     "Elapsed time: 1.694175 msecs"
Comment by Ghadi Shayban [ 28/Jun/14 6:47 PM ]

It's good that the benchmarks contain empty doseq bodies in order to isolate the overhead of traversal. However, that represents 0% of actual real-world code.

At least for the first benchmark (large chunked seq), adding in some tiny amount of work did not change results signifantly. Neither for (map str [a])

(range 10000000) =>  (map str [a])
core/doseq
"Elapsed time: 586.822389 msecs"
"Elapsed time: 563.640203 msecs"
"Elapsed time: 369.922975 msecs"
"Elapsed time: 366.164601 msecs"
"Elapsed time: 373.27327 msecs"
doseq2
"Elapsed time: 419.704021 msecs"
"Elapsed time: 371.065783 msecs"
"Elapsed time: 358.779231 msecs"
"Elapsed time: 363.874448 msecs"
"Elapsed time: 368.059586 msecs"

nor for intrisics like (inc a)

(range 10000000)
core/doseq
"Elapsed time: 317.091849 msecs"
"Elapsed time: 272.360988 msecs"
"Elapsed time: 215.501737 msecs"
"Elapsed time: 206.639181 msecs"
"Elapsed time: 206.883343 msecs"
doseq2
"Elapsed time: 241.475974 msecs"
"Elapsed time: 193.154832 msecs"
"Elapsed time: 198.757873 msecs"
"Elapsed time: 197.803042 msecs"
"Elapsed time: 200.603786 msecs"

I still see reduce-based doseq ahead of the original, except for small seqs

Comment by Ghadi Shayban [ 04/Aug/14 2:55 PM ]

A form like the following will not work with this patch:

(go (doseq [c chs] (>! c :foo)))

as the go macro doesn't traverse fn boundaries. The only such code I know is core.async/mapcat*, a private fn supporting a fn that is marked deprecated.

Comment by Ghadi Shayban [ 07/Aug/14 2:09 PM ]

I see #'clojure.core/run! was just added, which has a similar limitation

Comment by Rich Hickey [ 29/Aug/14 8:19 AM ]

Please consider Ghadi's feedback, esp re: closures.

Comment by Ghadi Shayban [ 22/Sep/14 4:36 PM ]

The current expansion of a doseq [1] under a go form is less than ideal due to the amount of control flow. 14 states in the state machine vs. 7 with loop/recur

[1] Comparison of macroexpansion of (go ... doseq) vs (go ... loop/recur)
https://gist.github.com/ghadishayban/639009900ce1933256a1

Comment by Nicola Mometto [ 25/Mar/15 6:07 PM ]

Related: CLJ-77

Comment by Nicola Mometto [ 03/Sep/15 12:59 PM ]

The general solution to this would be to automatically split methods when too large, using something like https://bitbucket.org/sperber/asm-method-size

Comment by Ghadi Shayban [ 24/Apr/16 12:25 AM ]

Example doseq impl and macroexpansion that does not suffer exponential bytecode growth. It also doesn't use any lambdas, so suitable for core.async.
https://gist.github.com/ghadishayban/fe84eb8db78f23be68e20addf467c4d4
It uses an explicit stack for the seqs/bindings.
It does not handle any chunking or modifiers yet.





[CLJ-1906] Clojure should make representing iterated api calls easier Created: 30/Mar/16  Updated: 23/Apr/16

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

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

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

 Description   

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

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

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

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

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

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

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

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

(import '(java.util UUID))

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

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

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

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

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

and the result would be true.

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

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


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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Now I'm partial to the name successions.





[CLJ-1916] AOT compilation sometimes results in extra classes for already compiled namespaces Created: 19/Apr/16  Updated: 19/Apr/16

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

Type: Defect Priority: Minor
Reporter: Mike Kaplinskiy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: aot


 Description   

Case-in-point: clojure/tools.logging.

Repro:

  • AOT compile all the namespaces in clojure/tools.logging (clojure.tools.logging & clojure.tools.logging.impl)
  • With the result on the classpath, AOT compile clojure/java.data (clojure.java.data)
  • Observe `clojure/tools/logging$eval32$fn__33.class` in the output of the second compile (make sure to have different output directories for the two compiles).

This is normally harmless, but becomes an issue if you try to cache AOT compilation output. When you try to cache previous AOT runs this way, you sometimes end up with two otherwise unrelated namespaces generating the same filename. If these had the same contents that would be fine, but there's no guarantee that they have the same contents (since 32 & 33 there are just (gensym)s). Depending on which one "wins" in a classpath this could end badly.

I'm not an expert here, but it would be nice if these "extras" were either generated as part of tools.logging or were somehow aliased into the namespace they were compiled from (e.g. clojure/java/data/$clojure$tools$logging$eval32$fn__33.class or clojure/tools/logging/$clojure$java$data$eval32$fn_33.class).



 Comments   
Comment by Kevin Downey [ 19/Apr/16 6:42 PM ]

tools.logging uses eval to generate some code only when certain classes are present on the classpath, eval generates class files, when you have aot compiling turned on, those class files will be output to the filesystem.

the reason the name is the way it is, is because the eval happens when the tools.logging namespace is loading, so the value of the ns var is the tools.logging namespace, which is what the compiler is generating the name from.





[CLJ-701] Primitive return type of loop and try is lost Created: 03/Jan/11  Updated: 19/Apr/16

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

Type: Defect Priority: Major
Reporter: Chouser Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None
Environment:

Clojure commit 9052ca1854b7b6202dba21fe2a45183a4534c501, version 1.3.0-master-SNAPSHOT


Attachments: Text File 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch     Text File 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch     Text File 0002-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch     File hoistedmethod-pass-1.diff     File hoistedmethod-pass-2.diff     File hoistedmethod-pass-3.diff     File hoistedmethod-pass-4.diff     File hoistedmethod-pass-5.diff     File hoistedmethod-pass-6.diff     File hoistedmethod-pass-7.diff    
Patch: Code and Test
Approval: Incomplete

 Description   
(set! *warn-on-reflection* true)
(fn [] (loop [b 0] (recur (loop [a 1] a))))

Generates the following warnings:

recur arg for primitive local: b is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: b

This is interesting for several reasons. For one, if the arg to recur is a let form, there is no warning:

(fn [] (loop [b 0] (recur (let [a 1] a))))

Also, the compiler appears to understand the return type of loop forms just fine:

(use '[clojure.contrib.repl-utils :only [expression-info]])
(expression-info '(loop [a 1] a))
;=> {:class long, :primitive? true}

The problem can of course be worked around using an explicit cast on the loop form:

(fn [] (loop [b 0] (recur (long (loop [a 1] a)))))

Reported by leafw in IRC: http://clojure-log.n01se.net/date/2011-01-03.html#10:31

See Also: CLJ-1422

Patch: 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch



 Comments   
Comment by a_strange_guy [ 03/Jan/11 4:36 PM ]

The problem is that a 'loop form gets converted into an anonymous fn that gets called immediately, when the loop is in a expression context (eg. its return value is needed, but not as the return value of a method/fn).

so

(fn [] (loop [b 0] (recur (loop [a 1] a))))

gets converted into

(fn [] (loop [b 0] (recur ((fn [] (loop [a 1] a))))))

see the code in the compiler:
http://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L5572

this conversion already bites you if you have mutable fields in a deftype and want to 'set! them in a loop

http://dev.clojure.org/jira/browse/CLJ-274

Comment by Christophe Grand [ 23/Nov/12 2:28 AM ]

loops in expression context are lifted into fns because else Hotspot doesn't optimize them.
This causes several problems:

  • type inference doesn't propagate outside of the loop[1]
  • the return value is never a primitive
  • mutable fields are inaccessible
  • surprise allocation of one closure objects each time the loop is entered.

Adressing all those problems isn't easy.
One can compute the type of the loop and emit a type hint but it works only with reference types. To make it works with primitive, primitie fns aren't enough since they return only long/double: you have to add explicit casts.
So solving the first two points can be done in a rather lccal way.
The two other points require more impacting changes, the goal would be to emit a method rather than a fn. So it means at the very least changing ObjExpr and adding a new subclassof ObjMethod.

[1] beware of CLJ-1111 when testing.

Comment by Alex Miller [ 21/Oct/13 10:28 PM ]

I don't think this is going to make it into 1.6, so removing the 1.6 tag.

Comment by Kevin Downey [ 21/Jul/14 7:14 PM ]

an immediate solution to this might be to hoist loops out in to distinct non-ifn types generated by the compiler with an invoke method that is typed to return the getJavaClass() type of the expression, that would give us the simplifying benefits of hoisting the code out and free use from the Object semantics of ifn

Comment by Kevin Downey [ 22/Jul/14 8:39 PM ]

I have attached a 3 part patch as hoistedmethod-pass-1.diff

3ed6fed8 adds a new ObjMethod type to represent expressions hoisted out in to their own methods on the enclosing class

9c39cac1 uses HoistedMethod to compile loops not in the return context

901e4505 hoists out try expressions and makes it possible for try to return a primitive expression (this might belong on http://dev.clojure.org/jira/browse/CLJ-1422)

Comment by Kevin Downey [ 22/Jul/14 8:54 PM ]

with hoistedmethod-pass-1.diff the example code generates bytecode like this

user=> (println (no.disassemble/disassemble (fn [] (loop [b 0] (recur (loop [a 1] a))))))
// Compiled from form-init1272682692522767658.clj (version 1.5 : 49.0, super bit)
public final class user$eval1675$fn__1676 extends clojure.lang.AFunction {
  
  // Field descriptor #7 Ljava/lang/Object;
  public static final java.lang.Object const__0;
  
  // Field descriptor #7 Ljava/lang/Object;
  public static final java.lang.Object const__1;
  
  // Method descriptor #10 ()V
  // Stack: 2, Locals: 0
  public static {};
     0  lconst_0
     1  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16]
     4  putstatic user$eval1675$fn__1676.const__0 : java.lang.Object [18]
     7  lconst_1
     8  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16]
    11  putstatic user$eval1675$fn__1676.const__1 : java.lang.Object [20]
    14  return
      Line numbers:
        [pc: 0, line: 1]

 // Method descriptor #10 ()V
  // Stack: 1, Locals: 1
  public user$eval1675$fn__1676();
    0  aload_0 [this]
    1  invokespecial clojure.lang.AFunction() [23]
    4  return
      Line numbers:
        [pc: 0, line: 1]
  
  // Method descriptor #25 ()Ljava/lang/Object;
  // Stack: 3, Locals: 3
  public java.lang.Object invoke();
     0  lconst_0
     1  lstore_1 [b]
     2  aload_0 [this]
     3  lload_1 [b]
     4  invokevirtual user$eval1675$fn__1676.__hoisted1677(long) : long [29]
     7  lstore_1 [b]
     8  goto 2
    11  areturn
      Line numbers:
        [pc: 0, line: 1]
      Local variable table:
        [pc: 2, pc: 11] local: b index: 1 type: long
        [pc: 0, pc: 11] local: this index: 0 type: java.lang.Object

 // Method descriptor #27 (J)J
  // Stack: 2, Locals: 5
  public long __hoisted1677(long b);
    0  lconst_1
    1  lstore_3 [a]
    2  lload_3
    3  lreturn
      Line numbers:
        [pc: 0, line: 1]
      Local variable table:
        [pc: 2, pc: 3] local: a index: 3 type: long
        [pc: 0, pc: 3] local: this index: 0 type: java.lang.Object
        [pc: 0, pc: 3] local: b index: 1 type: java.lang.Object

}
nil
user=> 
  

the body of the method __hoisted1677 is the inner loop

for reference the part of the bytecode from the same function compiled with 1.6.0 is pasted here https://gist.github.com/hiredman/f178a690718bde773ba0 the inner loop body is missing because it is implemented as its own IFn class that is instantiated and immediately executed. it closes over a boxed version of the numbers and returns an boxed version

Comment by Kevin Downey [ 23/Jul/14 12:43 AM ]

hoistedmethod-pass-2.diff replaces 901e4505 with f0a405e3 which fixes the implementation of MaybePrimitiveExpr for TryExpr

with hoistedmethod-pass-2.diff the largest clojure project I have quick access to (53kloc) compiles and all the tests pass

Comment by Alex Miller [ 23/Jul/14 12:03 PM ]

Thanks for the work on this!

Comment by Kevin Downey [ 23/Jul/14 2:05 PM ]

I have been working through running the tests for all the contribs projects with hoistedmethod-pass-2.diff, there are some bytecode verification errors compiling data.json and other errors elsewhere, so there is still work to do

Comment by Kevin Downey [ 25/Jul/14 7:08 PM ]

hoistedmethod-pass-3.diff

49782161 * add HoistedMethod to the compiler for hoisting expresssions out well typed methods
e60e6907 * hoist out loops if required
547ba069 * make TryExpr MaybePrimitive and hoist tries out as required

all contribs whose tests pass with master pass with this patch.

the change from hoistedmethod-pass-2.diff in this patch is the addition of some bookkeeping for arguments that take up more than one slot

Comment by Nicola Mometto [ 26/Jul/14 1:37 AM ]

Kevin there's still a bug regarding long/doubles handling:
On commit 49782161, line 101 of the diff, you're emitting gen.pop() if the expression is in STATEMENT position, you need to emit gen.pop2() instead if e.getReturnType is long.class or double.class

Test case:

user=> (fn [] (try 1 (finally)) 2)
VerifyError (class: user$eval1$fn__2, method: invoke signature: ()Ljava/lang/Object;) Attempt to split long or double on the stack  user/eval1 (NO_SOURCE_FILE:1)
Comment by Kevin Downey [ 26/Jul/14 1:46 AM ]

bah, all that work to figure out the thing I couldn't get right and of course I overlooked the thing I knew at the beginning. I want to get rid of some of the code duplication between emit and emitUnboxed for TryExpr, so when I get that done I'll fix the pop too

Comment by Kevin Downey [ 26/Jul/14 12:52 PM ]

hoistedmethod-pass-4.diff logically has the same three commits, but fixes the pop vs pop2 issue and rewrites emit and emitUnboxed for TryExpr to share most of their code

Comment by Kevin Downey [ 26/Jul/14 12:58 PM ]

hoistedmethod-pass-5.diff fixes a stupid mistake in the tests in hoistedmethod-pass-4.diff

Comment by Nicola Mometto [ 21/Aug/15 1:03 PM ]

Kevin Downey FWIW the patch no longer applies

Comment by Michael Blume [ 21/Aug/15 3:06 PM ]

A naive attempt to bring the patch up to date results in

compile-clojure:
     [java] Exception in thread "main" java.lang.ExceptionInInitializerError
     [java] 	at clojure.lang.Compile.<clinit>(Compile.java:29)
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7360)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7341)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
     [java] 	at clojure.lang.Compiler.access$300(Compiler.java:38)
     [java] 	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:589)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7112)
     [java] 	at clojure.lang.Compiler.eval(Compiler.java:7416)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7859)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:372)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:363)
     [java] 	at clojure.lang.RT.load(RT.java:453)
     [java] 	at clojure.lang.RT.load(RT.java:419)
     [java] 	at clojure.lang.RT.doInit(RT.java:461)
     [java] 	at clojure.lang.RT.<clinit>(RT.java:331)
     [java] 	... 1 more
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod
     [java] 	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4466)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7351)
     [java] 	... 17 more
Comment by Kevin Downey [ 25/Aug/15 1:00 PM ]

the pass 6 patch builds cleanly on 1d5237f9d7db0bc5f6e929330108d016ac7bf76c(HEAD of master as of this moment) and runs clojure's tests. I have not verified it against other projects as I did with the previous patches (I don't remember how I did that)

Comment by Michael Blume [ 25/Aug/15 1:38 PM ]

I get the same error I shared before applying the new patch to 1d5237f

Comment by Alex Miller [ 25/Aug/15 1:56 PM ]

I get some whitespace warnings with hoistedmethod-pass-6.diff but the patch applies. On a compile I get:

compile-clojure:
     [java] Exception in thread "main" java.lang.ExceptionInInitializerError
     [java] 	at clojure.lang.Compile.<clinit>(Compile.java:29)
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7362)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7156)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7343)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7156)
     [java] 	at clojure.lang.Compiler.access$300(Compiler.java:38)
     [java] 	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:588)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7355)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7156)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7114)
     [java] 	at clojure.lang.Compiler.eval(Compiler.java:7418)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7861)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:372)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:363)
     [java] 	at clojure.lang.RT.load(RT.java:453)
     [java] 	at clojure.lang.RT.load(RT.java:419)
     [java] 	at clojure.lang.RT.doInit(RT.java:461)
     [java] 	at clojure.lang.RT.<clinit>(RT.java:331)
     [java] 	... 1 more
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod
     [java] 	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4468)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353)
     [java] 	... 17 more
Comment by Nicola Mometto [ 25/Aug/15 2:36 PM ]

Looks like direct linking interacts with the diffs in this patch in non trivial ways.

Comment by Kevin Downey [ 25/Aug/15 3:05 PM ]

I must have screwed up running the tests some how, I definitely get the same error now

Comment by Kevin Downey [ 25/Aug/15 4:00 PM ]

after you get past the cast issuing (just adding some conditional logic there)

it looks like HoistedMethodInvocationExpr needs to be made aware of if it is emitting in an instance method or a static method, and do the right thing with regard to this pointers and argument offsets. this will likely require HoistedMethod growing the ability to be a static method (and maybe preferring static methods when possible).

if you cause HoistedMethod to set usesThis to true on methods that use it, then everything appears hunky-dory (if I ran the tests correctly), but this largely negates the new direct linking stuff, which is not good.

Comment by Kevin Downey [ 27/Aug/15 9:30 PM ]

hoistedmethod-pass-7.diff adds a single commit to hoistedmethod-pass-5.diff

the single commit changes hoisted methods to always be static methods, and adjusts the arguments in the invocation of the hoisted method based on if the containing function is a static/direct function or not.

again I haven't done the extended testing with this patch.

here is an example of what the hoisted methods look like

user> (println (disassemble (fn ^long [^long y] (let [x (try (/ 1 0) (catch Throwable t 0))] x))))                                                                       

// Compiled from form-init3851661302895745152.clj (version 1.5 : 49.0, super bit)                                                                                        
public final class user$eval1872$fn__1873 extends clojure.lang.AFunction implements clojure.lang.IFn$LL {                                                                
                                                                                                                                                                         
  // Field descriptor #9 Lclojure/lang/Var;                                                                                                                              
  public static final clojure.lang.Var const__0;                                                                                                                         
                                                                                                                                                                         
  // Field descriptor #11 Ljava/lang/Object;                                                                                                                             
  public static final java.lang.Object const__1;                                                                                                                         
                                                                                                                                                                         
  // Field descriptor #11 Ljava/lang/Object;                                                                                                                             
  public static final java.lang.Object const__2;                                                                                                                         
                                                                                                                                                                         
  // Method descriptor #14 ()V                                                                                                                                           
  // Stack: 2, Locals: 0                                                                                                                                                 
  public static {};                                                                                                                                                      
     0  ldc <String "clojure.core"> [16]                                                                                                                                 
     2  ldc <String "/"> [18]                                                                                                                                            
     4  invokestatic clojure.lang.RT.var(java.lang.String, java.lang.String) : clojure.lang.Var [24]                                                                     
     7  checkcast clojure.lang.Var [26]                                                                                                                                  
    10  putstatic user$eval1872$fn__1873.const__0 : clojure.lang.Var [28]                                                                                                
    13  lconst_1    
    14  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [34]                                                                                                  
    17  putstatic user$eval1872$fn__1873.const__1 : java.lang.Object [36]                                                                                                
    20  lconst_0                                                                                                                                                         
    21  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [34]                                                                                                  
    24  putstatic user$eval1872$fn__1873.const__2 : java.lang.Object [38]                                                                                                
    27  return                                                                                                                                                           
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
                                                                                                                                                                         
  // Method descriptor #14 ()V                                                                                                                                           
  // Stack: 1, Locals: 1                                                                                                                                                 
  public user$eval1872$fn__1873();                                                                                                                                       
    0  aload_0 [this]                                                                                                                                                    
    1  invokespecial clojure.lang.AFunction() [41]                                                                                                                       
    4  return                                                                                                                                                            
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
                                                                                                                                                                         
  // Method descriptor #43 (J)J                                                                                                                                          
  // Stack: 2, Locals: 4  
  public final long invokePrim(long y);                                                                                                                                  
     0  lload_1 [y]                                                                                                                                                      
     1  invokestatic user$eval1872$fn__1873.__hoisted1874(long) : java.lang.Object [47]                                                                                  
     4  astore_3 [x]                                                                                                                                                     
     5  aload_3 [x]                                                                                                                                                      
     6  aconst_null                                                                                                                                                      
     7  astore_3                                                                                                                                                         
     8  checkcast java.lang.Number [50]                                                                                                                                  
    11  invokevirtual java.lang.Number.longValue() : long [54]                                                                                                           
    14  lreturn                                                                                                                                                          
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
      Local variable table:                                                                                                                                              
        [pc: 5, pc: 8] local: x index: 3 type: java.lang.Object                                                                                                          
        [pc: 0, pc: 14] local: this index: 0 type: java.lang.Object                                                                                                      
        [pc: 0, pc: 14] local: y index: 1 type: long                                                                                                                     
                                                                                                                                                                         
  // Method descriptor #59 (Ljava/lang/Object;)Ljava/lang/Object;                                                                                                        
  // Stack: 5, Locals: 2                                                                                                                                                 
  public java.lang.Object invoke(java.lang.Object arg0);                                                                                                                 
     0  aload_0 [this]                                                                                                                                                   
     1  aload_1 [arg0]                                                                                                                                                   
     2  checkcast java.lang.Number [50]                                                                                                                                  
     5  invokestatic clojure.lang.RT.longCast(java.lang.Object) : long [63]                                                                                              
     8  invokeinterface clojure.lang.IFn$LL.invokePrim(long) : long [65] [nargs: 3]                                                                                      
    13  new java.lang.Long [30]                                                                                                                                          
    16  dup_x2                                                                                                                                                           
    17  dup_x2                                                                                                                                                           
    18  pop                                                                                                                                                              
    19  invokespecial java.lang.Long(long) [68]                                                                                                                          
    22  areturn                                                                                                                                                          
                                                                                                                                                                         
                                                                                                                                                                         
  // Method descriptor #45 (J)Ljava/lang/Object;                                                                                                                         
  // Stack: 4, Locals: 4                                                                                                                                                 
  public static java.lang.Object __hoisted1874(long arg0);                                                                                                               
     0  lconst_1                                                                                                                                                         
     1  lconst_0                                      
                                                                                                                                                        
     2  invokestatic clojure.lang.Numbers.divide(long, long) : java.lang.Number [74]                                                                                     
     5  astore_2                                                                                                                                                         
     6  goto 17                                                                                                                                                          
     9  astore_3 [t]                                                                                                                                                     
    10  getstatic user$eval1872$fn__1873.const__2 : java.lang.Object [38]                                                                                                
    13  astore_2                                                                                                                                                         
    14  goto 17                                                                                                                                                          
    17  aload_2                                                                                                                                                          
    18  areturn                                                                                                                                                          
      Exception Table:                                                                                                                                                   
        [pc: 0, pc: 6] -> 9 when : java.lang.Throwable                                                                                                                   
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
        [pc: 2, line: 1]                                                                                                                                                 
      Local variable table:                                                                                                                                              
        [pc: 9, pc: 14] local: t index: 3 type: java.lang.Object                                                                                                         
        [pc: 0, pc: 18] local: y index: 1 type: java.lang.Object                                                                                                         
                                                                                                                                                                         
}                           

Comment by Kevin Downey [ 27/Aug/15 9:43 PM ]

there is still an issue with patch 7, (defn f [^long y] (let [x (try (+ 1 0) (catch Throwable t y))] x)) causes a verifier error

Comment by Nicola Mometto [ 28/Aug/15 11:41 AM ]

Patch 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch is patch hoistedmethod-pass-7 with the following changes:

  • fixes the bug mentioned in the last comment by Kevin Downey
  • removes unnecessary whitespace and indentation changes
  • conforms indentation with the surrounding lines
  • squashes the commits into one as preferred by Rich

Authorship is maintained for Kevin Downey

Comment by Kevin Downey [ 28/Aug/15 5:40 PM ]

maybe we need a "Bronsa's Guide to Submitting Patches" to supplement http://dev.clojure.org/display/community/Developing+Patches, I had no idea a single commit was preferred, but that makes sense given the format, although I just noticed the bit on deleting old patches to avoid confusion. Is there a preferred format for patch names too?
If you have recommendations beyond patch formatting into the code itself I am all ears.

Comment by Nicola Mometto [ 28/Aug/15 6:08 PM ]

Kevin, having a single commit per patch is something that I've seen Rich and Alex ask for in a bunch of tickets, as I guess it makes it easier to evaluate the overall diff (even though it sacrifices granularity of description).
No idea for a preferred patch name, I find it hard to imagine it would matter – I just use whatever git format patch outputs.

One thing I personally prefer is to add the ticket name at the beginning of the commit message, it makes it easier to understand changes when using e.g. git blame

Comment by Andy Fingerhut [ 28/Aug/15 6:33 PM ]

Just now I added a suggestion to http://dev.clojure.org/display/community/Developing+Patches that one read their patches before attaching them, and remove any spurious white space changes. Also to consider submitting patches with a single commit, rather than ones broken up into multiple commits, as reviewers tend to prefer those.

Alex Miller recently edited that page with the note about putting the ticket id first in the commit comment.

Only preference on patch file names is the one on that page – that they end with '.patch' or '.diff', because Rich's preferred editor for reading them recognizes those suffixes and displays the file in a patch-specific mode, I would guess.

Comment by Nicola Mometto [ 28/Aug/15 6:38 PM ]

0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch is the same as 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch but it changes some indentation to avoid mixing tabs and spaces

Comment by Michael Blume [ 01/Sep/15 5:27 PM ]

I have a verify error on the latest patch, will attempt to provide a small test case:

Exception in thread "main" java.lang.VerifyError: (class: com/climate/scouting/homestead_test$fn__17125, method: invokeStatic signature: (Ljava/lang/Object;)Ljava/lang/Object Expecting to find integer on stack, compiling:(homestead_test.clj:43:3)

Comment by Michael Blume [ 01/Sep/15 6:29 PM ]

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

I'd rather remove the dependency on compojure-api but I'm having trouble reproducing without it

Comment by Michael Blume [ 01/Sep/15 8:01 PM ]

Ok, less stupid test case:

(let [^boolean foo true]
  (do
    (try foo
      (catch Throwable t))
    nil))
Comment by Michael Blume [ 01/Sep/15 8:30 PM ]

...I wonder how hard it would be to write a generative test which produced small snippets of valid Clojure code, compiled them, and checked for errors. I bet we could've caught this.

Comment by Nicola Mometto [ 02/Sep/15 12:53 AM ]

Michael Blume I'm not sure this should be considered a compiler bug.
Contrary to numeric literals, boolean literals are never emitted unboxed and type hints are not casts so

(let [^boolean a true])
is lying to the compiler telling it a is an unboxed boolean when infact it is a boxed Boolean.

I'd say we wait for what Alex Miller or Rich Hickey think of this before considering Kevin Downey's current patch bugged, I personally (given the current compiler implementation) would consider this an user-code bug, currently ignored, that this patch merely exposes. IOW an instance of currently working code that is not however valid code

(I rest my case that if we had some better spec on what type-hints are supposed to be valid and some better compile-time validation on them, issues like this would not arise)

Comment by Nicola Mometto [ 02/Sep/15 1:13 AM ]

If this is considered a bug, the fix is trivial btw

@@ -5888,7 +5888,7 @@ public static class LocalBinding{
 
 	public boolean hasJavaClass() {
 		if(init != null && init.hasJavaClass()
-		   && Util.isPrimitive(init.getJavaClass())
+		   && (Util.isPrimitive(init.getJavaClass()) || Util.isPrimitive(tag))
 		   && !(init instanceof MaybePrimitiveExpr))
 			return false;
 		return tag != null
Comment by Kevin Downey [ 03/Sep/15 11:22 AM ]

I imagine ^boolean type hints (that don't do anything and are ignored) are going to become very common in clojure code, given everyone's keen interest in code sharing between clojure and clojurescript, and iirc clojurescript actually using ^boolean

Comment by Nicola Mometto [ 12/Sep/15 5:33 AM ]

Last patch doesn't compile anymore since it hits the bug reported in CLJ-1809

Comment by Alex Miller [ 18/Sep/15 8:28 AM ]

Moving to incomplete for now since it seems to be blocked on the other ticket

Comment by Kevin Downey [ 19/Jan/16 4:06 PM ]

0002-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch makes the change that Nicola suggested (with an extra null check). Michael's test case with the ^boolean type hint compiles now

Comment by Michael Blume [ 19/Jan/16 6:01 PM ]

Maybe this is out of scope for this ticket since it's just existing code that you're moving around, but I've always been confused by

//exception should be on stack
gen.visitVarInsn(OBJECT_TYPE.getOpcode(Opcodes.ISTORE), finallyLocal);
finallyExpr.emit(C.STATEMENT, objx, gen);
gen.visitVarInsn(OBJECT_TYPE.getOpcode(Opcodes.ILOAD), finallyLocal);

If we emit finallyExpr as C.STATEMENT, it should have no impact on the stack, and the exception should just be there without us needing to store and load it, right?

Comment by Michael Blume [ 19/Jan/16 6:02 PM ]

Also the latest patch seems to add a patch file to the root directory

Comment by Michael Blume [ 19/Jan/16 7:01 PM ]

New failing code

(defn foo [req]
  (let [^boolean b (get req :is-baz)]
    (do
      (try
        :bar)
      :foo)))
Comment by Kevin Downey [ 20/Jan/16 12:05 AM ]

the latest patch applied to master is causing some test failures for data.xml

Testing clojure.data.xml.test-seq-tree

FAIL in (release-head-top) (test_seq_tree.clj:52)
expected: (= nil (.get input-ref))
  actual: (not (= nil (0 1 2 3 4 5 6 7 8 9)))

FAIL in (release-head-nested-late) (test_seq_tree.clj:60)
expected: (= nil (.get input-ref))
  actual: (not (= nil (1 2 :< 3 4 5 :>))
Comment by Nicola Mometto [ 20/Jan/16 5:54 AM ]

Michael Blume that's still an invalid type hint. Clojure is inconsistent all over the place with enforcing valid type hints, so even though I think we should handle the VerifyError explicitly, I don't think we need to care about making that code work.

WRT ^boolean type hints going to be common because clojurescript uses them – I don't think we should accept invalid code for the sake of interoparability. See CLJ-1883 for an instance of such a wrong type hint being used in Om, the solution was to fix Om, not to make the clojure compiler ignore yet another wrong type hint.

Comment by Kevin Downey [ 19/Apr/16 1:17 PM ]

I've just got around to trying to dig in to the data.xml failures I mentioned above.

Both those tests are related to head holding on a lazy seq. Both tests reliably fail with 0002-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch

So I suspect the hoisted method isn't properly clearing locals.

Comment by Kevin Downey [ 19/Apr/16 5:56 PM ]

so I don't forget, I realized the issue with data.xml is because the 'is' macro in the test expands in to a try/catch in an expression context, which is hoisted, and the hoist causes the whole environment not to be cleared until the hoisted method returns, which is obviously not correct.





[CLJ-1915] Tests for clojure.core/atom Created: 18/Apr/16  Updated: 18/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Linus Ericsson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: atom

Attachments: File 0001-atom-unit-tests.clj    
Patch: Code and Test
Approval: Triaged

 Description   

As per discussion with Alex Miller Mars 3rd 2016 on clojure-dev, Alex suggested we should add tests to clojure.core/atom functionality, of which there is none today.

I proposed tests for

  • the various ways to instatiate atoms (with and without validator and metadata)
  • that validators throws correctly
  • adding and removing watchers and that they trig as one would expected.
  • various ways of changing values (no aim for finding high-load concurrency issues or patological cases or similar).
  • that the arities of the interface in IAtom .swap works as expected - ie no reflection warnings (help/pointers for these type of cases needed!)
  • generative, tests trying to find the glitches while using atoms (the things excluded above).

Alex suggested generative testing, but no performance tests.

The patch "0001-atom-unit-tests.clj" (attached) contains unit tests for

  • creating "bare" atom
  • creating atom with validator
  • that validate-fn triggers and that the atom is unchanged
  • that deref (@) reader macro creates correct '(clojure.core/deref a)
  • that CAS works for ordinary values (no validator-triggering etc).

There are plenty of combinations not covered with these tests, but this is a start.

To cover all cases (like cas-ing with invalid values and other strange things) generative testing is indeed a must.






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

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

Type: Enhancement Priority: Minor
Reporter: Renzo Borgatti Assignee: Unassigned
Resolution: Unresolved Votes: 1
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: Prescreened

 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 but is widened to 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/master/src/jvm/clojure/lang/Compiler.java#L6270-L6282

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-1908] Add clojure.test api to run single test with fixtures and report Created: 01/Apr/16  Updated: 15/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: test

Attachments: Text File CLJ-1908-3.patch    
Patch: Code
Approval: Prescreened

 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-1914] Range realization has a race during concurrent execution Created: 14/Apr/16  Updated: 14/Apr/16

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

Type: Defect Priority: Critical
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: range

Attachments: Text File clj-1914-2.patch     Text File clj-1914-3.patch     Text File CLJ-1914.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

LongRange, which has some complex chunked seq caching, has a race in forceChunk.

chunkedMore, which calls forceChunk(), then checks for the existence of _chunkNext:

One thread calling forceChunk() can stop between these two lines:
https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LongRange.java#L131-L132

While another thread thinks the first is done
https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LongRange.java#L117

Thanks to Kyle Kingsbury for the reproducing case https://gist.github.com/aphyr/8746181beeac6a728a3aa018804d56f6

Approach: Swap order of setting _chunk and _chunkNext so _chunk serves as a gate after both pieces of state have been set.

Patch: clj-1914-3.patch



 Comments   
Comment by Alex Miller [ 14/Apr/16 11:13 PM ]

It's not necessary to synchronize here - just swapping these two lines should address the race I think: https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LongRange.java#L131-L132

Comment by Ghadi Shayban [ 14/Apr/16 11:22 PM ]

Good call. That's super subtle.

It appears all mutable assignment occurs in forceChunk() except for https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LongRange.java#L145 which should be OK (famous last words).





[CLJ-1913] core.reducers wrong documentation Created: 14/Apr/16  Updated: 14/Apr/16

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

Type: Defect Priority: Trivial
Reporter: Camilo Roca Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, reducers


 Description   

Two issues regarding the documentation of core.reducers

  • There is a contradiction between the documentation mentioned in http://clojure.github.io/clojure/clojure.core-api.html#clojure.core.reducers/fold, with respect to the one mentioned here http://clojure.org/reference/reducers. Specifically on the line that states "(with a seed value obtained by calling (combinef) with no arguments)" on the former and "The reducef function will be called with no arguments to produce an identity value in each partition." on the later. Those two documentation references are contradictory. Either combinef is called with no arguments or reducef is called with no arguments.
  • The second doc issue is regarding the arities of most functions in core.reducers. With the introduction of transducers in Clojure 1.7. The single arity in functions like r/map or r/filter gives the impression that they return a transducer, whereas they just return a curried version of them. Nothing in the docstrings or the reference page mentions what is the return value of those functions with a single argument.





[CLJ-1912] Optimized version of the '<' and '>' functions for arties larger than 2 Created: 08/Apr/16  Updated: 08/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Anton Harald Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance


 Description   

When looking at the code of the build-in functions '<' and '>', I was wondering, why (next more) is invoked twice in each comparison of two neighboring arguments.

Here is the original code of e.g. <

(defn <
  "Returns non-nil if nums are in monotonically increasing order,
  otherwise false."
  {:inline (fn [x y] `(. clojure.lang.Numbers (lt ~x ~y)))
   :inline-arities #{2}
   :added "1.0"}
  ([x] true)
  ([x y] (. clojure.lang.Numbers (lt x y)))
  ([x y & more]
   (if (< x y)
     (if (next more)
       (recur y (first more) (next more))
       (< y (first more)))
     false)))

Here is a possible replacement for the n-arity part of the function:

([x y & more]
   (if (< x y)
     (if-let [n (next more)]
       (recur y (first more) n)
       (< y (first more)))
     false))

Now, (next more) would be computed only once per 'iteration'. On my machine, the modified version had 7% better performance. Of course, this only shows up when invoked with more than 2 arguments. e.g.: (apply < (range 100000...))

I'd be curious to hear, if there was a particular reason for taking this decision in the built-in function.



 Comments   
Comment by Alex Miller [ 08/Apr/16 4:23 PM ]

I don't think there is a particular reason, feel free to make a patch.





[CLJ-1817] Allow AssertionError messages for function :pre and :post conditions to be specified. Created: 23/Sep/15  Updated: 03/Apr/16

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

Type: Enhancement Priority: Major
Reporter: Tristan Strange Assignee: Colin Taylor
Resolution: Unresolved Votes: 4
Labels: error-reporting
Environment:

All Clojure platforms


Attachments: Text File CLJ-1817.patch    
Patch: Code and Test
Approval: Triaged

 Description   

A failing in a predicate in a list of :pre or :post conditions currently causes messages similar to one below to be displayed:

(defn must-be-a-map [m] {:pre [(map? m)]} m)
(must-be-a-map [])
;;=> AssertionError Assert failed: (map? m)  user/must-be-a-map (form-init.....clj:1)

These exception messages could be made significantly more descriptive by allowing specific messages strings to be associated with each predicate in :pre and :post conditions.

Predicate functions and there associated messages strings could be specified as a pair of values in a map:

(defn must-be-a-map 
  [m]
  {:pre [{(map? m) "m must be a map due to some domain specific reason."}]}
  m)

The following would then produce an error message as follows:

(must-be-a-map 10)
AssertionError Assert failed: m must be a map due to some domain specific reason.
(map? m) user/must-be-a-map (form-init.....clj:1)

This would allow predicates without messages to specified alongside pairs of associated predicate message pairs as follows:

(defn n-and-m [n m] {:pre [(number? n) {(map? m) "You must provide a map!"}]})

This change would not break any existing functionality and still allow for predicates to be predefined elsewhere in code.

As a result pre and post conditions could provide a natural means of further documenting the ins and outs of a function, simplify the process of providing meaningful output when developing libraries and perhaps make the language better suited to teaching environments[1]

[1] http://wiki.science.ru.nl/tfpie/images/2/22/TFPIE2013_Steps_Towards_Teaching_Clojure.pdf



 Comments   
Comment by Colin Taylor [ 03/Apr/16 5:26 PM ]

Attached approach differs from that advocated for in the description by not requiring a map. The existing spec of :

{:pre [pre-expr*]
 :post [post-expr*]}

in effect becoming :

{:pre [(pre-expr assert-msg?)*]
 :post [(pre-expr assert-msg?)*]}

where assert-msg is a String. Note this means a (presumably erroneous) second String after an expression would be treated as a truthy pre-expr.

Contrived example :

(defn print-if-alphas-and-nums [arg] {:pre [(hasAlpha arg) "No alphas"
                                            (hasNum arg) "No numbers"
                                            (canPrint arg)]}
  (println arg))

user=> (print-if-alphas-and-nums "a5%")
a5%
nil
user=> (print-if-alphas-and-nums "$$%")
AssertionError Assert failed: No alphas
(hasAlpha arg)  user/print-if-alphas-and-nums (NO_SOURCE_FILE:19)

I have considered extending the spec further to (pre-expr assert-msg? data-map)* perhaps supported by ex-info, ex-data analogues in assert-info, assert-data to convey diagnostic info (locals?). A map could contain a :msg key or perhaps the map is additional to the message string. I thought I'd wait for input though at this point.

I also considered allowing % substitution for the fn return value in the message as in :post conds, but how to escape?

Comment by Colin Taylor [ 03/Apr/16 6:17 PM ]

I should point out that the tests include the currently uncovered existing functionality too.





[CLJ-1243] Cannot resolve public generic method from package-private base class Created: 01/Aug/13  Updated: 03/Apr/16

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

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

Attachments: GZip Archive clj-1243-demo1.tar.gz    

 Description   

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

Instructions to reproduce:

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

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

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

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



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

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

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

But why does it only happen on generic methods?

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

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

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

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

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

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

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

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

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

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

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

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

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





[CLJ-1791] Issue defining a defrecord protocol method named "clear" Created: 04/Aug/15  Updated: 01/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

There seems to be a problem in trying to define a protocol with a method named "clear"

(defprotocol PClear
(clear [o]))
=> PClear

(defrecord Foo []
PClear
(clear [o] o))
=> CompilerException java.lang.ClassFormatError: Duplicate method name&signature in class file xxxx/Foo, compiling:(NO_SOURCE_PATH:1:1)

I assume this is due to a name conflict with the Java method Collection.clear() in the underlying implementation. However the error is very unclear about this, and the potential for conflict appears to be undocumented as far as I can see.

There seem to be two possible approaches to fixing this:
a) Disallow the use of "clear" as a protocol method name (in which case the error should be more informative, and the rule should be documented)
b) Find a way to support this in the class file format (possibly by overloading on JVM return types, since Collection.clear() returns void??)



 Comments   
Comment by Nicola Mometto [ 04/Aug/15 6:58 AM ]

Mike, the jvm doesn't support return type overloading so your second suggestion is not technically possible.

Reading the doc for defrecord

The class will have implementations of several (clojure.lang)
interfaces generated automatically: IObj (metadata support) and
IPersistentMap, and all of their superinterfaces.

Perharps java.util.Collection (or even better, java.util.Map) should be mentioned here.

Comment by Alex Miller [ 04/Aug/15 7:46 AM ]

I think this should be a doc enhancement request.

Comment by OHTA Shogo [ 08/Aug/15 3:42 AM ]

It might be out of the scope of this ticket, but protocol method conflicts can cause some other kinds of errors:

user=> (defprotocol P1 (finalize [this]))
P1
user=> (defrecord R1 [] P1 (finalize [this]))

CompilerException java.lang.VerifyError: (class: user/R1, method: finalize signature: ()Ljava/lang/Object;) Unable to pop operand off an empty stack, compiling: ...
user=> (defprotocol P2 (wait [this]))
P2
user=> (defrecord R2 [] P2 (wait [this]))
user.R2
user=> (def r (->R2))
#'user/r
user=> (wait r)
CompilerException java.lang.IllegalArgumentException: No single method: wait of interface: user.P2 found for function: wait of protocol: P2, compiling: ...
user=>

IMHO it would be nicer if defprotocol would warn method conflicts with a more informative message.

Comment by Mike Anderson [ 31/Mar/16 7:53 PM ]

@Nicola Mometto : I believe the JVM does in fact support return type overloading:

"Note that there may be more than one matching method in a class because while the Java language forbids a class to declare multiple methods with the same signature but different return types, the Java virtual machine does not. This increased flexibility in the virtual machine can be used to implement various language features. For example, covariant returns can be implemented with bridge methods; the bridge method and the method being overridden would have the same signature but different return types."

See : http://docs.oracle.com/javase/8/docs/api/java/lang/Class.html#getMethod-java.lang.String-java.lang.Class...-

Comment by Nicola Mometto [ 01/Apr/16 3:55 AM ]

Ah, yes of course, thanks.





[CLJ-1530] Make foo/bar/baz unreadable Created: 22/Sep/14  Updated: 31/Mar/16

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: 0
Labels: reader

Attachments: Text File 0001-fix-LispReader-and-EdnReader-so-that-foo-bar-baz-is-.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Currently keywords and symbols containing more than one slash are disallowed by the spec, but allowed by the readers.
This trivial patch makes them unreadable by the readers too.

Pre:

user=> :foo/bar/baz
:foo/bar/baz

Post:

user=> :foo/bar/baz
RuntimeException Invalid token: :foo/bar/baz  clojure.lang.Util.runtimeException (Util.java:221)


 Comments   
Comment by Andy Fingerhut [ 22/Sep/14 12:14 PM ]

Perhaps overlap with CLJ-1527 ?

Comment by Thomas Engelschmidt [ 28/Oct/14 4:36 AM ]

Please notice that keywords with more than one slash has a different hashcode across clojure version 1.5 and 1.6

This creates a problem when using a datomic version that works with clojure 1.5 under clojure 1.6 and the schema have one or more keys with more than one slash.

Comment by Chris Zheng [ 30/Mar/16 8:41 AM ]

Please reconsider this `fix` for the following reasons:

Please see discussion on this topic below:

dm3 [5:04 PM]
Is there a reason why `(read-string "a/b/c")` is OK, while `(clojure.tools.reader/read-string "a/b/c")` fails with `Invalid token`?

hiredman [5:04 PM]
there is a ticket to fix read-string

dm3 [5:05 PM]
so the correct behaviour is to fail?

hiredman [5:05 PM]
http://dev.clojure.org/jira/browse/CLJ-1530

dm3 [5:06 PM]
thanks, seems like a breaking change :simple_smile:

hiredman [5:07 PM]
the docs have precluded symbols like a/b/c for some time, and the behavior of how those unspecified symbols were read apparently changed at some point

lucas [5:09 PM]
joined #clojure

zcaudate [5:16 PM]
@hiredman: seriously. I’m really peeved about that because I’ve been using `:a/b/c` keywords for a while… even wrote a whole freaking library to deal with that http://docs.caudate.me/hara/hara-string.html#api---path (edited)

[5:16]
and now they are taking it out

[5:18]
i’m of the opinion that the `::foo/baz` keywords should be taken out first

[5:20]
```user=> (require '[clojure.walk :as walk])
nil
user=> ::walk/hello
:clojure.walk/hello
```

[5:20]
that causes way more problems

[5:20]
especially with analysers

[5:22]
https://github.com/jonase/kibit/issues/14

GitHub
Kibit breaks on namespace qualified keywords · Issue #14 · jonase/kibit · GitHub
If the code contains namespace qualified keywords with aliases, Kibit errors out with a Invalid token exception. The following code demonstrates the problem - ;;; foo.clj (ns foo) ;;; bar.clj (n...

[5:25]
@dm3 if there’s really a problem, you can patch it:

[5:25]
https://github.com/helpshift/hydrox/blob/master/lein/src/leiningen/hydrox/setup.clj

GitHub
helpshift/hydrox
hydrox - Dive deeper into your code

dm3 [5:31 PM]
yeah, would have to patch cljs.tools.reader too unfortunately :confused:

[5:31]
think I'll just work around that

bronsa [5:36 PM]
@zcaudate: `::foo/bar` style keywords are in by design and not going anywhere, `:foo/bar/baz` have always been invalid by the spec and undefined behaviour (edited)

[5:37]
@dm3: is changing undefined behaviour a breaking change? :simple_smile:

dm3 [5:37 PM]
breaking as in breaking people's code :simple_smile:

[5:37]
e.g. zcaudate

bronsa [5:38 PM]
that code is already broken if it's using invalid clojure. It's just accidentaly working

dm3 [5:39 PM]
I'm looking from a pragmatic perspective. Theoretically you're right :simple_smile:

[5:40]
and I'm not judging either

bronsa [5:40 PM]
pragmatically, `:foo/bar/baz` is a bug waiting to happen. what does `(namespace :foo/bar/baz)` return?

dm3 [5:40 PM]
whatever it returns currently?

[5:41]
I mean it's kind of defined by the implementation

sveri [5:41 PM]
@dm3 @bronsa I wouldnt even agree that you are theoretically right. As soon as enough people adapted the broken code it falls under something like a common law that was accepted by both parties for a long enough time.

bronsa [5:42 PM]
what about `namespace` on `(keyword "foo/bar" "baz")` and `(keyword "foo" "bar/baz")`

dm3 [5:43 PM]
I agree that currently implemented semantics are messy

[5:43]
but my point was that it's still a breaking change

[5:43]
not that it's a "bad" change

[5:43]
that's a judgement

bronsa [5:43 PM]
@sveri: I would agree with you, as long as the undefined behaviour we accept as defined doesn't cause impossible to fix semantics.

[5:44]
that's why for example, the patch that made symbols starting with numbers illegal was rolled back

[5:45]
it broke existing code, it didn't cause weird semantics so it was rolled back. not the case with `:foo/bar/baz`

[5:45]
@dm3: you could make the point that fixing any bug is a breaking change then – people might be relying on that bug.

dm3 [5:46 PM]
yep, I guess what matters is how obvious the incorrect behaviour is and how many people rely on it

bronsa [5:46 PM]
if the doc explicitely says "you can use ​one​ `/` inside a symbol", then if you're using more than one you're writing invalid clojure and you should expect it to maybe break (edited)

dm3 [5:47 PM]
I really haven't even thought about multiple slashes (nor noticed the docs) in a symbol in ~3 years of using Clojure

[5:47]
my initial thought (today) was that it was permitted

[5:47]
and the namespace would be the first segment before the first slash (edited)

bronsa [5:51 PM]
well, that doesn't make much sense though. `/` in clojure means `namespace separator`. if I see `FOO/BAR`, no matter what `FOO` and `BAR` are, I know that `FOO` is the namespace, and `BAR` is the name. if you want to express paths with keywords as in @zcaudate's lib, you should use a different separator in your keywords that doesn't have a special meaning in clojure, like `.` (i.e. `:foo/bar/baz` -> `:foo.bar.baz` or `:foo/bar.baz`) (edited)

dm3 [5:51 PM]
I do not want to argue semantics. Just sharing one point of view

bronsa [5:52 PM]
and my point is that pragmatic point of views (especially when they go against the current doc) should only be considered if the semantics they imply are clear and unambiguous

sveri [5:53 PM]
@bronsa: Nice explanation, thank you :simple_smile:

dm3 [5:53 PM]
yep :simple_smile:

[5:54]
I agree to that, as you have to make decisions in the end

zcaudate [9:07 PM]
@bronsa: written form of communication has a way of making things more serious than they seem

[9:08]
honestly… i knew it was coming since 1.6 when the edn reader started breaking my code

[9:09]
it’s probably more my fault for not communicating this earlier but oh well.. we all have to roll with the times

bronsa [9:09 PM]
@zcaudate: yeah no worries, I was just using your lib as an example since you brought that up

zcaudate [9:10 PM]
having said that, you can imagine my disappointment because I had designed an entire query semantic based on the keyword `:foo/bar/baz` feature (now bug) (edited)

[9:11]
http://docs.caudate.me/adi/adi-walkthrough.html#querying

[9:11]
you noticed I didn’t use `(adi/select ds {:student/classes/teacher/name "Mr. Blair"}})`

[9:12]
in my docs because the tests started breaking

jstew [9:12 PM]
@zcaudate: You put out so much quality stuff that I wonder if you ever sleep!
1

bronsa [9:12 PM]
@zcaudate: luckly the fix should be easy :simple_smile: just replace `/` with `.`

zcaudate [9:12 PM]
no!

[9:13]
see the problem is… datomic has things like `account.type/user`

[9:13]
and so I would have to do the cljs thing `account.type$user`

bronsa [9:13 PM]
(that's some high quality documentation btw, good job)

zcaudate [9:14 PM]
@bronsa: hahaha thanks… so maybe you can push the fix to 1.10

[9:14]
that way I can get a few more months left

[9:15]
like it’s not a big deal… but I thought that there is a parallel between the path structure of the `/` calls and the nesting of maps

[9:16]
and so there is an equivalence to `{:student {:classes {:teacher {:name '(?fulltext "Blair")}`

[9:17]
and `{:student/classes/teacher/name "Mr. Blair"}`

[9:17]
which is prettier in my opinion

bronsa [9:17 PM]
@zcaudate: sorry if that wasn't clear, but I don't actually have any control over when or what gets into clojure or not, I'm just a contributor :simple_smile: so there are chances that the clojure/core team will take a different decision and actually decline that ticket (I would be really disappointed if that was the case though!). If that will happenI will obviously make a change to `tools.reader` to allow them aswell, (edited)

zcaudate [9:18 PM]
@bronsa: damn.

[9:19]
well… maybe you can highlight this fact

[9:19]
and also if the fix is made, a fix to `(keyword "foo/bar/baz")` will also be needed (edited)

bronsa [9:20 PM]
don't think that'll ever be done. validating inputs to `keyword`/`symbol` etc has been asked/discussed tons of times and repeatedly declined for performance reasons

zcaudate [9:20 PM]
so that’s a matter of consistency.

bronsa [9:20 PM]
(not that I agree with that decision, but it seems like Rich isn't going ot change his mind on that)

[9:21]
@zcaudate: there's a difference between what a symbol/keyword can be at runtime, and what a valid read-time symbol/keyword is

zcaudate [9:21 PM]
and also, it means I can setup a reader macro #k foo/bar/baz and get the same effect

[9:22]
like it’s stupidly ugly but i believe it will work

bronsa [9:23 PM]
but the ambiguity of what to do with `namespace` and `name` still remains so dunno

[9:23]
@zcaudate: that wouldn't work either way, if http://dev.clojure.org/jira/browse/CLJ-1530 gets accepted neither `:foo/bar/baz` ​nor​ `foo/bar/baz` will be valid anymore

[9:24]
@zcaudate: btw I'd suggest you log your issues with that ticket in a comment there if you feel strongly against it

new messages
[9:25]
I ​suspect​ that the response will be "you should use a delimiter that doesn't have a special meaning in clojure", but I might be completely wrong (I find the core team doesn't agree with my opinions quite frequently ) especially if you point out that your library will break. (edited)

zcaudate [9:30 PM]
@bronsa: thanks for the heads up. I’ll leave a comment and add a prayer for the bdfl

Comment by Alex Miller [ 30/Mar/16 8:55 AM ]

Chris -

  • (keyword "foo/bar/baz") will still be fine. Programmatic keywords can be created for any string - this is intentionally much broader than what the reader supports as a literal in code and it's a feature that's widely used. At some future point, there may be an escaping mechanism for symbols or keywords with characters outside the spec such that the reader could read them as well, but that's outside the scope of this.
  • Your api is using illegal keywords according to http://clojure.org/reference/reader and you should not expect them to work. I think you should change your library.
Comment by Chris Zheng [ 30/Mar/16 6:15 PM ]

Alex

  • I wouldn't necessarily call it `illegal` as the current behavior in the edn.reader was added in 1.6 without warning.
  • Also, if (keyword "foo/bar/baz") is allowed to exist, then there still would be the indeterminate namespace/name problem that @bronsa highlighted. I would argue for consistency and if :foo/bar/baz is illegal in the reader, then it should be illegal everywhere
  • My library should be fine... but users of the library may have to change their queries
Comment by Alex Miller [ 30/Mar/16 7:00 PM ]

The reader page clearly states "'/' has special meaning, it can be used once in the middle of a symbol to separate the namespace from the name" and keywords are "like symbols". This has been on the reader page since the oldest version I can find in the internet archive (July 2008). edn (despite its similarities) is a separate thing than Clojure, and irrelevant.

The 2-arity form of keyword can be used without ambiguity: (keyword nil "foo/bar/baz"). The 1-arity form will split based on the first / found (in this example into "foo" and "bar/baz"). I see no reason that would need to change.

Comment by Chris Zheng [ 31/Mar/16 8:58 AM ]

I disagree on the "specialness" of `/` in keywords, especially if it is allowed as a string.

This leads to another problem in the ambiguity of output:

user=> (keyword "foo" "bar/baz")
:foo/bar/baz

user=> (keyword nil "foo/bar/baz")
:foo/bar/baz

Unless the output is displayed as :foo//bar/baz, it is unclear where the namespace is and the concept of `code is data` will be diminished if the output cannot be read back as data.

There is also the case for symbol:
user=> (symbol "foo/bar/baz")
foo/bar/baz

well... at least all the examples listed are all consistently "illegal"

Anyways, even if the docs had not explicitly stated such, :foo/bar/baz has existed since the beginning of clojure and personally, it seems to be more string-like than symbol-like. Ironically, I'm pretty sure that I got the idea of using multiple slashes in keywords from reading the datomic documentation from back when I started work on adi.

Ultimately, the decision is not mine to make and I do value the guidance of clojure team over the development of the language. I do however, hope that my points for keeping things as they are can be recognised and be taken into consideration.

Comment by Chris Zheng [ 31/Mar/16 9:23 AM ]

I'll quote Rich https://www.youtube.com/watch?v=P76Vbsk_3J0 @ 5:10

"Many of the things you consider to be problems (with lisp) are features... down the line..."





[CLJ-1907] Document non-caching behaviour of `iterate` when used as generator Created: 31/Mar/16  Updated: 31/Mar/16

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Approval: Triaged

 Description   

The non-caching behaviour of `iterate` when used as a generator is not documented and counter-intuitive. It should be documented, just like it's documented for e.g. `eduction`.

Even though the docstring for `iterate` requires `f` to be side-effect free, `f` might take a long time to compute, in which case users should be wary that the computation might happen more than once.






[CLJ-1793] Clear 'this' before calls in tail position Created: 05/Aug/15  Updated: 30/Mar/16

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

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

1.8.0-alpha2 - 1.8.0-alpha4


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

 Description   

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

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

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

Example of regression from CLJ-1250 that now works correctly (doesn't clear this in nested loop) with CLJ-1793:

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

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

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

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

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

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

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

Patch: clj-1793-2.patch

Screened by: Alex Miller



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

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

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

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

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

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

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

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

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

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

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

See the commit message for an extensive-ish comment.

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

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

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

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

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

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

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

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

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

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

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

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

Before patch:

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

After patch:

user=> (defn range* [x] (cons x (lazy-seq (range* (inc x)))))
#'user/range*
user=> (reduce + 0 (eduction (range* 0)))
;; runs infinitely without causing OOM




[CLJ-1314] Correct placement of doc string for function bubble-max-key Created: 23/Dec/13  Updated: 29/Mar/16

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

Type: Enhancement Priority: Trivial
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: File clj-1314-v2.diff    
Patch: Code
Approval: Prescreened

 Description   

It is private, defined with defn-, so perhaps the doc string is superfluous, but someone wrote one, and it is in the wrong place relative to the args:

(defn- bubble-max-key [k coll]
  "Move a maximal element of coll according to fn k (which returns a number) 
   to the front of coll."
  (let [max (apply max-key k coll)]
    (cons max (remove #(identical? max %) coll))))

Patch: clj-1314-v2.diff
Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 23/Dec/13 4:11 PM ]

Patch clj-1314-v1.diff simply corrects the location of the doc string for bubble-max-key.

Comment by Andy Fingerhut [ 02/Jan/14 11:05 AM ]

Patch clj-1314-v2.diff is same as the previous clj-1314-v1.diff, except it leaves out some unintended changes.





[CLJ-1360] clojure.string/split strips trailing delimiters Created: 18/Feb/14  Updated: 24/Mar/16

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

Type: Defect Priority: Minor
Reporter: Tim McCormack Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: docstring, string

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

 Description   

clojure.string/split and clojure.string/split-lines inherit the bizarre default behavior of java.lang.String#split(String,int) in stripping trailing consecutive delimiters:

(clojure.string/split "banana" #"an")
⇒ ["b" "" "a"]
(clojure.string/split "banana" #"na")
⇒ ["ba"]
(clojure.string/split "nanabanana" #"na")
⇒ ["" "" "ba"]

In the case of split-lines, processing a file line by line and rejoining results in truncation of trailing newlines in the file. In both cases, the behavior is surprising and cannot be inferred from the docstrings. A workaround for split is to pass a limit of -1.

Proposed: As current users may be relying on the current behavior, the attached merely updates the docstring to warn of this behavior and suggest use of -1 as a limit to workaround.

Patch: clj-1360.patch



 Comments   
Comment by Andy Fingerhut [ 18/Feb/14 10:51 AM ]

Probably documenting would be safer than changing the behavior at this point, given that some people may actually rely on the current behavior after testing, deploying, etc.

I don't currently have a suggestion for a modified doc string, but note that there are examples of this behavior and how one can use an extra "-1" limit argument at the end to get all split strings: http://clojuredocs.org/clojure_core/clojure.string/split

Comment by Crispin Wellington [ 21/May/15 10:46 PM ]

This bug just bit me. +1 to be fixed. If we just document and leave the behavior as is, then we have a surprising and inconsistent behaving split (why are inner empty values kept, but outer ones stripped?) that is different to every other split you've ever used. The optional -1 limit argument looks hacky but a fix could keep this -1 argument working.

EDIT: this looks to be java's string class behavior: http://stackoverflow.com/questions/2170557/split-method-of-string-class-does-not-include-trailing-empty-strings
Would be nice if limit defaulted to -1 on that type of clojure.string/split call.

Comment by Stuart Halloway [ 19/Jul/15 8:03 AM ]

This is really gross, and the original developer has been punched in the neck. (Ow.)

I hate the Java leakage, but given that this is already out there, and that people are likely already relying on both the default and the negative-arg behavior, I think the least bad bet is to document precisely the semantics we have.





[CLJ-1279] Fix confusing macroexpand1 ArityException handling Created: 16/Oct/13  Updated: 24/Mar/16

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

Type: Defect Priority: Major
Reporter: Alex Coventry Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: Compiler, errormsgs, macro

Attachments: Text File 0001-Edit-macro-ArityException-in-AFn.patch     Text File 0001-Fix-macroexpand1-s-handling-of-ArityException.patch    
Patch: Code and Test
Approval: Triaged

 Description   

macros can give very confusing error messages when they execute a form which generates an ArityException. clojure.lang.Compiler.macroexpand1 assumes that any ArityException comes from the call to the macro itself, which need not be the case. For instance:

user> (do (defmacro f [] (assoc)) (f))
  ArityException Wrong number of args (-2) passed to: core$assoc  clojure.lang.Compiler.macroexpand1 (Compiler.java:6488)
  user> (use 'clojure.repl) (pst)
  nil
  ArityException Wrong number of args (-2) passed to: core$assoc
  	clojure.lang.Compiler.macroexpand1 (Compiler.java:6488)
  	clojure.lang.Compiler.macroexpand (Compiler.java:6544)
  	clojure.lang.Compiler.eval (Compiler.java:6618)
  	clojure.lang.Compiler.eval (Compiler.java:6624)
  	clojure.lang.Compiler.eval (Compiler.java:6597)
  	clojure.core/eval (core.clj:2864)
  	clojure.main/repl/read-eval-print--6596/fn--6599 (main.clj:260)
  	clojure.main/repl/read-eval-print--6596 (main.clj:260)
  	clojure.main/repl/fn--6605 (main.clj:278)
  	clojure.main/repl (main.clj:278)
  	clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn--1251 (interruptible_eval.clj:56)
  	clojure.core/apply (core.clj:617)
  nil

Easy enough to see the source of the problem in this case, but because both the number of arguments actually passed is off by two, and the stacktrace element for the call to assoc has been dropped, this shortcut by macroexpand1 can get super confusing.

The attached patch corrects this behavior. E.g.

user=> (do (defmacro f [] (assoc)) (f))
  ArityException Wrong number of args (0) passed to: core$assoc  clojure.lang.AFn.throwArity (AFn.java:437)
  user=> (use 'clojure.repl) (pst)
  nil
  ArityException Wrong number of args (0) passed to: core$assoc
  	user/f (NO_SOURCE_FILE:1)
  	clojure.lang.Var.invoke (Var.java:419)
  	clojure.lang.Var.applyTo (Var.java:532)
  	clojure.lang.Compiler.macroexpand1 (Compiler.java:6507)
  	clojure.lang.Compiler.macroexpand (Compiler.java:6580)
  	clojure.lang.Compiler.eval (Compiler.java:6654)
  	clojure.lang.Compiler.eval (Compiler.java:6660)
  	clojure.lang.Compiler.eval (Compiler.java:6633)
  	clojure.core/eval (core.clj:2864)
  	clojure.main/repl/read-eval-print--6594/fn--6597 (main.clj:260)
  	clojure.main/repl/read-eval-print--6594 (main.clj:260)
  	clojure.main/repl/fn--6603 (main.clj:278)
  nil


 Comments   
Comment by Alex Coventry [ 17/Oct/13 11:01 AM ]

Patch with test

Comment by Alex Coventry [ 23/Oct/13 11:42 PM ]

Amended patch to deal more gracefully with unexpected stack trace structure.

Comment by Alex Miller [ 24/Oct/13 12:09 AM ]

Also see CLJ-397 and CLJ-383.

Comment by Alex Coventry [ 24/Oct/13 2:46 PM ]

Thanks, Alex. It would be easy enough to move most of the logic into ArityException, which would be a compromise between Stu's[1] options 1 and 2. Is that worth doing?

Amending clojure.lang.AFn.throwArity to check whether "this" is a macro and adjust the arg count there accordingly might be the simplest way. I can see why Rich prefers all the logic to go into ArityException, but since ArityExceptions are used for things other than macros, I don't see a way to make an honest error message there without groveling the stack trace.

[1] http://dev.clojure.org/jira/browse/CLJ-397?focusedCommentId=24090&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-24090

Comment by Alex Miller [ 24/Oct/13 2:57 PM ]

I would have to take more time than I have to make an informed opinion but I can say that from a general point of view inspecting StackTraceElements does not seem like the right solution to (almost) any problem.

Comment by Alex Coventry [ 24/Oct/13 10:26 PM ]

This patch causes Var.setMacro to set instance attribute AFn.macrop to true, so that AFn.throwArity can reduce the number of arguments reported.

I'm not used to negotiating java class hierarchies, so it's possible there's a cleaner way. Since Var.fn() returns an IFn, I added macrop handling methods IFn.setMacro and IFn.isMacro. These then needed to be implemented in Ref and Keyword, as well as AFn (where I wanted them) because they implement the IFn interface but don't inherit from AFn.

The real drawback I see with this approach is the duplicated state, though: ^{:macro true} vs AFn.macrop==true.

Comment by Andy Fingerhut [ 25/Oct/13 6:33 PM ]

I have not investigated the reason yet, but neither patch applies cleanly after the latest commits to Clojure master on Oct 25 2013. Given that what kinds of solution methods would be acceptable for this issue, it sounds like more thinking and code changes are probably needed anyway before worrying too much about that.





[CLJ-1777] Add function version of vswap! Created: 13/Jul/15  Updated: 19/Mar/16

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File 0001-CLJ-1777-Change-vswap-from-a-macro-to-an-inlineable-.patch     Text File 0001-CLJ-1777-Change-vswap-from-a-macro-to-an-inlineable-v2.patch    
Patch: Code

 Description   

Volatiles and vswap! were added in 1.7 as performant mechanisms to achieve uncoordinated mutation to the language.
given the fact that their addition was a performance-centric one, vswap! was implemented as a macro rather than a normal function to avoid runtime dereference of the function var and the optional apply overhead in case of multiple args.

However this:
-is not necessary
-breaks the api parallelism between volatile/atom swap!/vswap! reset!/vreset!
-makes impossible certain use cases (vswaps! in update-in forms)
-is potentially confusing given that swap! is a function

Infact the macro can be replaced with a function with :inline metadata.
This is a strictly additive change that will make so that for all the current valid usages of vswap! nothing will change, it will still be macroexpanded by the inliner and additionally since it is now a function it can be used in HOF contexts where it's not unusual to see swap! used.



 Comments   
Comment by Alex Miller [ 13/Jul/15 8:07 PM ]

Nicola, please don't set the fix version on tickets.

Comment by Nicola Mometto [ 13/Jul/15 8:10 PM ]

Sorry, I mixed the fix version with the affected field

Comment by Greg Chapman [ 09/Mar/16 11:41 AM ]

An additional drawback to the current macro is that it will result in double evaluation of the vol expression:

user=> (macroexpand '(vswap! (return-a-vol) inc))
(. (return-a-vol) reset (inc (.deref (return-a-vol))))
Comment by Alex Miller [ 10/Mar/16 9:22 AM ]

I don't think Rich is interested in expanding the use of inline.

Re Greg's comment, that would be considered if you want to make a second ticket.

Comment by Nicola Mometto [ 10/Mar/16 12:31 PM ]

Not really sure I understand the adversion to using inline – it works perfectly fine and this is the exact use-case for it: a function that we want to inline for performance reason.

I really fail to see the reasoning behind making what should be a function, a macro, just for the sake of not using inline.

Comment by Phill Wolf [ 19/Mar/16 8:59 AM ]

Adding :inline to existing functions may be deplored as an unprincpled, sloppy, desperate, tragic compromise, and a slippery slope. But vswap! is the opposite case: The macro is what already exists, somewhat idiosyncratically. Pairing it with a function would make it more wholesome, not less.





[CLJ-1903] Provide a transducer for reductions Created: 17/Mar/16  Updated: 18/Mar/16

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

Type: Enhancement Priority: Major
Reporter: Pierre-Yves Ritschard Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: transducers

Attachments: Text File 0001-clojure.core-add-reductions-stateful-transducer.patch     Text File 0002-clojure.core-add-reductions-with-for-init-passing-va.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Reductions does not currently provide a transducer when called with a 1-arity.

Proposed:

  • A reductions transducer
  • Similar to seequence reductions, initial state is not included in reductions
(assert (= (sequence (reductions +) nil) []))
(assert (= (sequence (reductions +) [1 2 3 4 5]) [1 3 6 10 15]))

A second patch proposes a variant which allows explicit initialization values: reductions-with

(assert (= (sequence (reductions-with + 0) [1 2 3 4 5]) [1 3 6 10 15])))

Patch: 0001-clojure.core-add-reductions-stateful-transducer.patch
Patch: 0002-clojure.core-add-reductions-with-for-init-passing-va.patch



 Comments   
Comment by Steve Miner [ 17/Mar/16 3:47 PM ]

The suggested patch gets the "init" value for the reductions by calling the function with no args. I would like a "reductions" transducer that took an explicit "init" rather than relying on a nullary (f).

If I remember correctly, Rich has expressed some regrets about supporting reduce without an init (ala Common Lisp). My understanding is that an explicit init is preferred for new Clojure code.

Unfortunately, an explicit init arg for the transducer would conflict with the standard "no-init" reductions [f coll]. In my own code, I've used the name "accumulations" for this transducer. Another possible name might be "reductions-with".

Comment by Pierre-Yves Ritschard [ 17/Mar/16 4:38 PM ]

Hi Steve,

I'd much prefer for init values to be explicit as well, unfortunately, short of testing the 2nd argument in the 2-arity variant - which would probably be even more confusing, there's no way to do that with plain "reductions".

I like the idea of providing a "reductions-with" variant that forced the init value and I'm happy to augment the patch with that if needed.

Comment by Pierre-Yves Ritschard [ 18/Mar/16 3:35 AM ]

@Steve Miner I added a variant with reductions-with.





[CLJ-99] max-key and min-key evaluate k multiple times for arguments Created: 17/Jun/09  Updated: 16/Mar/16

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

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: performance

Attachments: File clj-99-v1.diff     Text File clj-99-v2.patch     Text File clj-99-v3.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

max-key or min-key will evaluate (k value) multiple times for arguments if more than 2 arguments are passed. This is undesirable if k is expensive to calculate.

Good perf test:

(apply max-key #(Math/tan %) (range 1000))

Approach: Avoid repeated evaluation of k for any element.

A criterium test of the example above shows:

  • Before: 206.017411 µs
  • After: 126.532306 µs

Patch: clj-99-v3.patch



 Comments   
Comment by Assembla Importer [ 24/Aug/10 5:45 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 5:45 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Andy Fingerhut [ 15/Nov/12 9:36 PM ]

clj-99-min-key-max-key-performance-v1.txt dated Nov 15 2012 changes min-key and max-key to evaluate the function k on each of its other arguments at most once.

Comment by Andy Fingerhut [ 22/Oct/13 7:54 PM ]

Patch clj-99-v1.diff is same as previously attached patch, but with .diff suffix for easier diff-style viewing in some editors.

Comment by Steve Kim [ 11/Apr/14 10:48 AM ]

sort-by is similarly inefficient. It calls keyfn for every comparison in sort, not once for every element to be sorted.

I'm not familiar with this project's preferred workflow, so I have to ask: Do you want me to open a separate ticket for sort-by, or do you prefer to consolidate that issue into this ticket?

Comment by Alex Miller [ 11/Apr/14 10:50 AM ]

Separate.

Comment by Steve Kim [ 11/Apr/14 11:47 AM ]

I created CLJ-1402 for sort-by

Comment by Michael Blume [ 09/Feb/15 2:50 PM ]

clj-99-v2 applies cleanly to master

Comment by Michael Blume [ 03/Nov/15 7:52 PM ]

clj-99-v3.patch behaves correctly for collection elements that are nil or false.





[CLJ-1117] partition docstring should be more explicit about dropped or partial trailing elements Created: 29/Nov/12  Updated: 16/Mar/16

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

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

OS X, 10.8


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

 Description   

The doc for partition states "In case there are not enough padding elements, return a partition with less than n items." However, the behavior of this function is as follows:

user=> (partition 3 (range 10))
((0 1 2) (3 4 5) (6 7 8))
user=> (partition 4 (range 10))
((0 1 2 3) (4 5 6 7))

Proposed: Proposed docstring update in bold:

Returns a lazy sequence of lists of n items each, at offsets step
apart. If step is not supplied, defaults to n, i.e. the partitions
do not overlap. **If a pad collection is not supplied, any tail elements
that remain from dividing the input collection length by n will not be
included in a partition.** If a pad collection is supplied, use its elements as
necessary to complete last partition upto n items. In case there are
not enough padding elements, return a partition with less than n items.

Patch: clj-1117.patch



 Comments   
Comment by Andy Fingerhut [ 29/Nov/12 2:15 PM ]

That would be a potentially breaking change for some people's code that uses partition. partition-all behaves as you wish.

Also, your concern with the documentation is for when there are padding elements specified as an argument, but your examples don't specify any padding elements.

Comment by Timothy Baldridge [ 29/Nov/12 2:55 PM ]

I agree, but I think the docs should then explicitly state: "if no padding is given, not all input elements may be returned in the output partitions" or something to that line.

Comment by Andy Fingerhut [ 29/Nov/12 4:43 PM ]

More precise documentation of current behavior is always welcome in my opinion.

Comment by Gabriel Horner [ 17/May/13 10:14 AM ]

I've uploaded a patch that calls out when and how partition drops tail elements:
"If a pad collection is not supplied, any tail elements that remain from dividing the input collection length by n will not be included in a partition."

Comment by Andrea Richiardi [ 11/Apr/15 10:38 AM ]

I was a little bit baffled by this as well...in particular, I could not figure out why (partition 2 3 [1 2 3 4 5 6 7]) is not '((1 2) (4 5) (7))...it would really be better to rewrite that comment.





[CLJ-1888] AReference#meta() is synchronized Created: 26/Jan/16  Updated: 16/Mar/16

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

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

 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-1899] Add function transform-keys to clojure.walk Created: 08/Mar/16  Updated: 14/Mar/16

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

Type: Enhancement Priority: Major
Reporter: Rafal Szalanski Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: walk
Environment:

OS X, Java 8, Clojure 1.8


Attachments: Text File clj1899.patch     Text File clj1899-review1.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

In CLJ-1894 I proposed a patch to change clojure.walk/stringify-keys to include namespace if keywords use namespaces. I made a wrong assumption about backwards compatibility of that change, however I still think the behaviour is not exactly what it should be.

Interesting thing Alex Miller pointed out in his comment to CLJ-1894 is that stringify-keys and keywordize-keys are essentially the same function with a different transformation. I think having one function which does a deep transformation of map keys using a transformation supplied by user is a good idea and it could be used to simplify some Clojure libraries.

Proposal:

  • add clojure.walk/transform-keys to walk a map and transform all keys
  • use transform-keys in clojure.walk/stringify-keys & clojure.walk/keywordize-keys

Patch: clj1899-review1.patch

Screened by: Alex Miller



 Comments   
Comment by Rafal Szalanski [ 08/Mar/16 6:37 AM ]

CLJ-1899 patch

Comment by Alex Miller [ 10/Mar/16 9:20 AM ]

In the patch, transform-keys should take the arguments in the reverse order [m f] - generally for any function that is collection -> collection, the collection should be the first arg.

Comment by Rafal Szalanski [ 14/Mar/16 9:34 AM ]

CLJ-1899 patch addressing issues pointed by Alex miller.





[CLJ-1673] Improve clojure.repl/dir-fn to work on namespace aliases in addition to canonical namespaces. Created: 11/Mar/15  Updated: 11/Mar/16

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

Type: Enhancement Priority: Minor
Reporter: Jason Whitlark Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ft, repl

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

 Description   

Extend clojure.repl/dir to work with the aliases in the current namespace

After:

user=> (require '[clojure.string :as s])
nil
user=> (dir s)
blank?
capitalize
...etc

Patch: clj-1673-3.patch

Screened by: Alex Miller



 Comments   
Comment by Jason Whitlark [ 11/Mar/15 4:00 PM ]

Possible unit test, since clojure.string is aliased in the test file:

(is (= (dir-fn 'clojure.string) (dir-fn 'str)))

Comment by Alex Miller [ 29/Apr/15 2:12 PM ]

Please add a test...

Comment by Jason Whitlark [ 02/May/15 10:35 PM ]

Updated patch, tweaked dir-fn, added test.

Comment by Jason Whitlark [ 02/May/15 10:38 PM ]

Should be good to go. I removed the old patch.

Comment by Alex Miller [ 04/May/15 4:38 PM ]

Tweaked patch just to remove errant blank line, otherwise same.

Comment by Michael Blume [ 12/Oct/15 5:54 PM ]

Applied this patch directly to master, test failure:

[java] ERROR in (test-dir) (core.clj:4023)
     [java] expected: (= (dir-fn (quote clojure.string)) (dir-fn (quote str)))
     [java]   actual: java.lang.Exception: No namespace: str found
     [java]  at clojure.core$the_ns.invokeStatic (core.clj:4023)
     [java]     clojure.repl$dir_fn.invokeStatic (repl.clj:186)
     [java]     clojure.repl$dir_fn.invoke (repl.clj:-1)
     [java]     clojure.test_clojure.repl$fn__21967$fn__21976.invoke (repl.clj:28)
     [java]     clojure.core$with_redefs_fn.invokeStatic (core.clj:7211)
     [java]     clojure.core$with_redefs_fn.invoke (core.clj:-1)
     [java]     clojure.test_clojure.repl$fn__21967.invokeStatic (repl.clj:27)
     [java]     clojure.test_clojure.repl/fn (repl.clj:-1)
     [java]     clojure.test$test_var$fn__7962.invoke (test.clj:703)
     [java]     clojure.test$test_var.invokeStatic (test.clj:703)
     [java]     clojure.test$test_var.invoke (test.clj:-1)
     [java]     clojure.test$test_vars$fn__7984$fn__7989.invoke (test.clj:721)
     [java]     clojure.test$default_fixture.invokeStatic (test.clj:673)
     [java]     clojure.test$default_fixture.invoke (test.clj:-1)
     [java]     clojure.test$test_vars$fn__7984.invoke (test.clj:721)
     [java]     clojure.test$default_fixture.invokeStatic (test.clj:673)
     [java]     clojure.test$default_fixture.invoke (test.clj:-1)
     [java]     clojure.test$test_vars.invokeStatic (test.clj:717)
     [java]     clojure.test$test_all_vars.invokeStatic (test.clj:723)
     [java]     clojure.test$test_ns.invokeStatic (test.clj:744)
     [java]     clojure.test$test_ns.invoke (test.clj:-1)
     [java]     clojure.core$map$fn__4770.invoke (core.clj:2639)
     [java]     clojure.lang.LazySeq.sval (LazySeq.java:40)
     [java]     clojure.lang.LazySeq.seq (LazySeq.java:49)
     [java]     clojure.lang.Cons.next (Cons.java:39)
     [java]     clojure.lang.RT.next (RT.java:688)
     [java]     clojure.core$next__4327.invokeStatic (core.clj:64)
     [java]     clojure.core$reduce1.invokeStatic (core.clj:924)
     [java]     clojure.core$reduce1.invokeStatic (core.clj:914)
     [java]     clojure.core$merge_with.invokeStatic (core.clj:2943)
     [java]     clojure.core$merge_with.doInvoke (core.clj:-1)
     [java]     clojure.lang.RestFn.applyTo (RestFn.java:139)
     [java]     clojure.core$apply.invokeStatic (core.clj:647)
     [java]     clojure.test$run_tests.invokeStatic (test.clj:754)
     [java]     clojure.test$run_tests.doInvoke (test.clj:-1)
     [java]     clojure.lang.RestFn.applyTo (RestFn.java:137)
     [java]     clojure.core$apply.invokeStatic (core.clj:645)
     [java]     clojure.core$apply.invoke (core.clj:-1)
     [java]     user$eval29133.invokeStatic (run_test.clj:8)
     [java]     user$eval29133.invoke (run_test.clj:-1)
     [java]     clojure.lang.Compiler.eval (Compiler.java:6934)
     [java]     clojure.lang.Compiler.load (Compiler.java:7381)
     [java]     clojure.lang.Compiler.loadFile (Compiler.java:7319)
     [java]     clojure.main$load_script.invokeStatic (main.clj:275)
     [java]     clojure.main$script_opt.invokeStatic (main.clj:335)
     [java]     clojure.main$script_opt.invoke (main.clj:-1)
     [java]     clojure.main$main.invokeStatic (main.clj:421)
     [java]     clojure.main$main.doInvoke (main.clj:-1)
     [java]     clojure.lang.RestFn.invoke (RestFn.java:408)
     [java]     clojure.lang.Var.invoke (Var.java:379)
     [java]     clojure.lang.AFn.applyToHelper (AFn.java:154)
     [java]     clojure.lang.Var.applyTo (Var.java:700)
     [java]     clojure.main.main (main.java:37)
Comment by Jason Whitlark [ 12/Oct/15 7:22 PM ]

After poking at it for a while, I discovered that the test in question isn't being executed in clojure.test-clojure.repl namespace, but in user. So replacing the failing test with:
(is (= (the-ns 'clojure.test-clojure.repl) ns))
also fails.

Which is very surprising to me. Is this the expected behavior? I could fix the test by doing the following:

(binding [*ns* (the-ns 'clojure.test-clojure.repl)]
(is (= (dir-fn 'clojure.string) (dir-fn 'str))))

But I don't want to mask an error, if this behavior is unexpected. Guidance please.

Comment by Alex Miller [ 12/Oct/15 7:57 PM ]

I don't want this to get applied as is so moving to incomplete for now.

Comment by Alex Miller [ 12/Oct/15 8:00 PM ]

You know this could be due to direct linking - calls inside the clojure code to other clojure code are now direct linked (static) calls as of 1.8.0-alpha3 and can't be redef'ed.

Comment by Alex Miller [ 12/Oct/15 8:03 PM ]

Yeah, if you turn direct linking off the test passes.

Comment by Jason Whitlark [ 13/Oct/15 1:06 PM ]

Fixed test to work with direct linking.

Comment by Jason Whitlark [ 10/Nov/15 12:58 AM ]

This is ready to go. Do I need to do anything to get it back in the queue?

Comment by Alex Miller [ 10/Nov/15 7:10 AM ]

We've missed the window for 1.8 but it's in the queue for 1.9.





[CLJ-1451] Add take-until Created: 20/Jun/14  Updated: 11/Mar/16

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

Type: Enhancement Priority: Critical
Reporter: Alexander Taggart Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: transducers

Attachments: Text File 0001-CLJ-1451-add-take-until.patch     Text File 0002-CLJ-1451-add-drop-until.patch     Text File 0003-let-take-until-and-drop-until-return-transducers.patch     Text File CLJ-1451-drop-until.patch     Text File clj-1451.patch     Text File CLJ-1451-take-until.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

Discussion: https://groups.google.com/d/topic/clojure-dev/NaAuBz6SpkY/discussion

It comes up when I would otherwise use (take-while pred coll), but I need to include the first item for which (pred item) is false.

(take-while pos? [1 2 0 3]) => (1 2)
(take-until zero? [1 2 0 3]) => (1 2 0)

Patch: clj-1451.patch

  • Includes transducer arity of take-until
  • Includes inclusion in transducer generative tests

Prescreened by: Alex Miller



 Comments   
Comment by Alex Miller [ 20/Jun/14 10:21 AM ]

Patch welcome (w/tests).

Comment by Alexander Taggart [ 20/Jun/14 2:00 PM ]

Impl and tests for take-until and drop-until, one patch for each.

Comment by Jozef Wagner [ 20/Jun/14 3:01 PM ]

Please change :added metadata to "1.7".

Comment by Alexander Taggart [ 20/Jun/14 3:12 PM ]

Updated to :added "1.7"

Comment by John Mastro [ 21/Jun/14 6:26 PM ]

I'd like to propose take-through and drop-through as alternative names. I think "through" communicates more clearly how these differ from take-while and drop-while.

Comment by Andy Fingerhut [ 06/Aug/14 2:27 PM ]

Both patches CLJ-1451-drop-until.patch and CLJ-1451-take-until.patch dated Jun 20 2014 no longer apply cleanly to latest Clojure master due to some changes committed earlier today. I haven't checked whether they are straightforward to update, but would guess that they merely require updating a few lines of diff context.

See the section "Updating stale patches" at http://dev.clojure.org/display/community/Developing+Patches for suggestions on how to update patches.

Comment by Ghadi Shayban [ 13/Nov/14 11:19 PM ]

Would be nice to cover the transducer case too.

Comment by Michael Blume [ 13/Nov/14 11:54 PM ]

rerolled patches

Comment by Michael Blume [ 14/Nov/14 12:11 AM ]

Covered transducer case =)

Comment by Michael Blume [ 14/Nov/14 12:12 AM ]

Actually I like take/drop-through as well

Comment by Ghadi Shayban [ 16/Nov/14 12:41 PM ]

Michael, no volatile/state is necessary in the transducer, like take-while. Just wrap in 'reduced to terminate

Comment by Michael Blume [ 17/Dec/14 6:47 PM ]

a) you're clearly right about take-until

b) seriously I don't know what I was thinking with my take-until implementation, I'm going to claim lack of sleep.

c) I'm confused about how to make drop-until work without a volatile

Comment by Michael Blume [ 18/Dec/14 1:52 AM ]

Ghadi and I discussed this and couldn't think of a use case for drop-until. Are there any?

Here's a new take-until patch, generative tests included.

Open questions:

Is take-until a good name? My biggest concern is that take-until makes it sound like a slight modification of take, but this function reverses the sense of the predicate relative to take.

Comment by Andy Fingerhut [ 08/Jan/15 6:06 PM ]

Michael, while JIRA can handle multiple attachments for the same ticket with the same name, it can get confusing for people trying to determine which one with the same name is meant. Could you remove or rename one of your identically-named attachments? Instructions for deleting patches are in the "Removing patches" section on this wiki page: http://dev.clojure.org/display/community/Developing+Patches

Comment by Alex Miller [ 11/Mar/16 2:49 PM ]

The patch was slightly stale so I updated to apply to master, but it's almost identical. Attribution retained.

Marked as prescreened.





[CLJ-706] make use of deprecated namespaces/vars easier to spot Created: 05/Jan/11  Updated: 11/Mar/16

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

Type: Enhancement Priority: Critical
Reporter: Stuart Halloway Assignee: Cezary Kosko
Resolution: Unresolved Votes: 19
Labels: errormsgs

Attachments: File 706-deprecated-ns-var-warnings-tested-2.diff     File 706-deprecated-ns-var-warnings-tested-3.diff     File 706-deprecated-ns-var-warnings-tested.diff     File 706-deprecated-var-warning.diff     Text File 706-deprecated-var-warning-patch-v2.txt     File 706-fix-deprecation-warnings-agents.diff     File 706-fix-deprecation-warnings-on-replicate.diff     File 706-fix-deprecation-warning-test-junit.diff     File 706-warning-on-deprecated-ns.diff    
Patch: Code and Test
Approval: Prescreened

 Description   

From the mailing list http://groups.google.com/group/clojure/msg/c41d909bd58e4534. It is easy to use deprecated namespaces or vars without knowing you are doing so. The documentation warnings are small, and there is no compiler warning.

Proposed:

  • Add new *warn-on-deprecated* dynamic var, defaulted to false
  • Warn to stderr when {:deprecated true} namespace is loaded.
  • Warn to stderr when {:deprecated true} var is analyzed.
  • Warn to stderr when {:deprecated true} macro is expanded.
  • New system property clojure.compiler.warn-on-deprecated
  • Compile Clojure itself with clojure.compiler.warn-on-deprecated
  • Fix deprecation warnings inside Clojure (replicate, clear-agent-errors)
  • Mark clojure.parallel as deprecated with :deprecation tag

Examples:

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

;; use of a deprecated var (on compile)
(defn ^:deprecated f [x] x)
(f 5)
;;=> Deprecation warning, NO_SOURCE_PATH:7:1 : var #'user/f is deprecated

;; use of a deprecated macro (on macro expansion)
(defmacro ^:deprecated m [x] x)
(m 5)
;;=> Deprecation warning, NO_SOURCE_PATH:7:1 : macro #'user/m is deprecated

;; use of a deprecated namespace (on load)
(ns foo {:deprecated "1.1"})
(ns bar (:require foo))
;;=> Deprecation warning: loading deprecated namespace `foo` from namespace `bar`

Patch: 706-deprecated-ns-var-warnings-tested-3.diff

Questions: Should default for deprecation warnings be true instead? People upgrading are likely to see new warnings which might be surprising.

  • Should default be to warn or not warn on deprecated?


 Comments   
Comment by Rich Hickey [ 07/Jan/11 9:38 AM ]

I don't mind warning to stderr

Comment by Luke VanderHart [ 26/Oct/12 1:37 PM ]

706-deprecated-var-warning.diff adds warnings when using deprecated vars. The other three patches clean up deprecation warnings.

Comment by Andy Fingerhut [ 26/Oct/12 2:23 PM ]

Great stuff. I looked through the first patch, and didn't see anything in there that lets someone disable deprecation warnings from the command line, the way that warn-on-reflection can today be set to true with a command line option.

Is that something important to have for deprecation warnings?

Comment by Andy Fingerhut [ 28/Oct/12 4:57 PM ]

I was hoping it would be quick and easy to add source file, line, and column info to the deprecation warning messages. It isn't as easy as adding them to the format() call, because the method analyzeSymbol doesn't receive these values as args. Is this deprecation check being done in a place where it is not easy to relate it to the source file, line, and column? Could it be done in a place where that info is easily available?

Comment by Ghadi Shayban [ 29/Oct/12 1:02 AM ]

Another patch - this time to warn on loading deprecated namespaces, instead of vars. This patch requires the first one.

Re: line/column, I'll figure out how to thread the compile context through if it's desired.

Re: Compile flag. I have a patch for this also, but I'm still verifying how to invoke. How is warn-on-reflection set by command line?

Comment by Andy Fingerhut [ 29/Oct/12 1:43 AM ]

Re: Compile flag. Don't hold off on implementing a flag if you want to, but it might be worth hearing from others whether such a command line option is even desired. I was asking in hopes of eliciting such a response.

For the way that it is handled in the Clojure compiler, search for REFLECTION_WARNING_PROP and related code in Compile.java. If you invoke the Clojure compiler directly via a Java command line, use -Dclojure.compile.warn-on-reflection=true (default is false). See the recent email thread sent to Clojure Dev Google group if you want to know how to do it via ant or Maven. Link: https://mail.google.com/mail/?shva=1#label/clojure-dev/13aa0e34530196c3

There is also a separate command-line flag called compiler-options (see Compile.java) that is implemented as a map inside the compiler. It was added more recently than warn-on-reflection, and might be the preferred method to add more such options, to avoid having to continue to add more arguments to the pushThreadBindings calls done in several places.

Comment by Ghadi Shayban [ 29/Oct/12 10:36 AM ]

Thanks, Andy.

Alternately for the last ns patch, it is equivalent to call (print-method msg err), rather than binding out to err, may be more readable. I'll be glad to send that in if it's preferable.

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

706-deprecated-var-warning-patch-v2.txt dated Feb 12 2013 is identical to 706-deprecated-var-warning.diff dated Oct 26 2012, except it applies cleanly to latest master.

Comment by Andy Fingerhut [ 23/Feb/15 8:21 PM ]

For the information of anyone examining this ticket wishing for this feature, the Eastwood lint tool reports calls to deprecated Clojure functions, and also to deprecated Java methods. https://github.com/jonase/eastwood

Comment by Alex Miller [ 25/Jan/16 12:32 PM ]

I'm interested in considering this for Clojure 1.9 but I need some help getting it ready. Some comments I have on the current state: - Ticket needs to have more details about the current approach

  • I prefer *warn-on-deprecated* over *warn-on-deprecation* because it echoes the keyword you use to mark deprecated vars
  • The warning message does not tell you a location, which is grr - should be similar to the reflection messages
  • Needs tests - see test/clojure/test_clojure/compilation.clj and test/clojure/test_helper.clj (should-not-reflect) for examples
  • clojure itself has some instances of deprecated usage - it would be nice to clean those up in the patch too. That may need to be in a separate patch, depends if they are easy to fix or not. If there are cases in test/ that are actually good to leave, can set *warn-on-deprecated* to false in that namespace.
  • Current default is true - should probably be false instead to match the reflection warning default.
Comment by Vijay Kiran [ 26/Jan/16 3:10 AM ]

Alex Miller I can give this a shot.

Comment by Alex Miller [ 26/Jan/16 8:51 AM ]

Hey Vijay, Andrew Rosa assigned it to himself so please coordinate with him as he was starting to work on it.

Comment by Bozhidar Batsov [ 26/Jan/16 10:52 AM ]

Just one small remark - isn't it more common to have deprecation warnings enabled by default? One could argue they are way more important than reflection warnings, as your code might get broken in the future because you didn't notice you were using deprecated stuff.

Comment by Alex Miller [ 26/Jan/16 2:01 PM ]

Bozhidar Batsov I'm on the fence. My main hesitation in making it the default is that people will suddenly have a bunch of new warnings (which could be either good or bad I suppose). Depends how strongly we want people to care about deprecations I guess.

Comment by Phill Wolf [ 26/Jan/16 9:33 PM ]

A deprecation warning that is off by default does not address the first and primary problem given in this ticket: "It is easy to use deprecated namespaces without knowing you are doing so."

It's unlike the reflection warning. You may focus on speed at any time, at your leisure. But the eventual removal of at-risk features will be a sudden, unpleasant surprise; a warning would be helpful.

But - Suppose I wrote 300 lines of Clojure and use a million lines that come from jars. Will any deprecation problems in my own code be buried in a tsunami of warnings about those jars? Worse, the tsunami will likely linger for weeks or months, until the libraries' respective authors catch up. Inasmuch as the jars are covered (much more expediently) by 'lein ancient' and similar, I would prefer to be able to limit deprecation warnings to just my stuff, perhaps by namespace prefix if from-a-jar-or-not is inconvenient from the compiler's point of view.

Comment by Alex Miller [ 26/Jan/16 10:35 PM ]

There is a middle ground here to turn it off by default in the compiler, but to turn it on by default in the tools (like lein). But there's a reasonable chance that whatever I prefer, Rich will have a preference that overrules it when it gets to him.

I think creating more complexity around namespace prefixes is unlikely to help this ticket move forward.

Comment by Cezary Kosko [ 07/Mar/16 9:18 PM ]

Uploaded a patch that coalesces var/ns-related patches by Luke & adds tests.
The patch does not, however, warn the user about deprecated macros, I assume I should adjust it, then?

Also, I'm not able adjust the description, so how do I take care of Alex's list's first bullet?

Comment by Andy Fingerhut [ 08/Mar/16 1:05 AM ]

Cezary, I have bumped up your permissions on JIRA so you should be able to edit tickets now. Please reload the page and try again.

Comment by Alex Miller [ 10/Mar/16 10:54 AM ]
  • The if in the first change in core.clj should be a when instead.
  • Can namespace deprecation warning include more about where this is occurring?
  • I'm having a hard time reproducing the deprecated ns warning in a manual test (see below). There seems to be something weird about the binding+printf as the conditions appear to be satisfied. I'm thinking it has something to do with flushing *err*? Seems like (println "Warning: loading deprecated ns" lib) would be better there.
(set! *warn-on-deprecated* true)
(ns foo {:deprecated true})
(ns bar (:require foo))
  • src/jvm/clojure/lang/Compile.java needs added support for clojure.compile.warn-on-deprecated RT flag
  • I think we should turn on warn-on-deprecated in the Clojure build itself (in build.xml)
  • If you do that, the following deprecation warnings exist in the Clojure build itself and we should fix those:

[java] Deprecation warning, clojure/core_proxy.clj:112:75 : var #'clojure.core/replicate is tagged as deprecated
[java] Deprecation warning, clojure/genclass.clj:149:41 : var #'clojure.core/replicate is tagged as deprecated
[java] Deprecation warning, clojure/genclass.clj:235:65 : var #'clojure.core/replicate is tagged as deprecated
[java] Deprecation warning, clojure/test/junit.clj:118:22 : var #'clojure.test/file-position is tagged as deprecated

  • Mark clojure.parallel as deprecated in the ns meta
Comment by Cezary Kosko [ 11/Mar/16 5:40 AM ]

Uploaded a new diff addressing the comments & added warning on macroexpansion.

As far as the namespace deprecation warning goes, though, the code's only printing the current namespace, did not know whether there was a decent way to get a file/line combo.

Comment by Alex Miller [ 11/Mar/16 8:53 AM ]

One more (hopefully final) round and then I think we're good:

  • The docstring for warn-on-deprecated should be updated now that we've expanded scope to cover ns too.
  • In the deprecated ns warning message, can we make it: "Deprecation warning: loading deprecated namespace `foo` from namespace `bar`."
  • In the macro and var warnings can we change "is tagged as deprecated" to just "is deprecated"?
  • Clean up the hanging parens in test/clojure/test_clojure/compilation/deprecated.clj

Thanks for the work on this!!

Comment by Alex Miller [ 11/Mar/16 1:49 PM ]

Marking pre-screened for Rich to look at.





[CLJ-1454] Add deref-swap! and deref-reset! (swap! and reset! that return prior value) Created: 28/Jun/14  Updated: 10/Mar/16

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

Type: Enhancement Priority: Critical
Reporter: Philip Potter Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: atom

Attachments: File deref-swap-deref-reset-extending-IAtom.diff     File deref-swap-deref-reset-with-IAtomDeref-and-tests.diff    
Patch: Code and Test
Approval: Prescreened

 Description   

Sometimes, when swapping or resetting an atom, it's desirable to know the value before the update. The existing swap! and reset! functions return the new value instead. Currently, the only option is to roll your own using a loop and compare-and-set!

Example use cases:

  • When an atom contains a PersistentQueue and you want to atomically remove the head of the queue and process it: if you run (swap! q pop), you have lost the reference to the old head of the list so you can't process it.
  • Want to check if an operation has occurred before by using atom as a flag (this can be achieved with compare-and-set! but reads a little easier this way).
    (def has-run-once (atom false))
    ...
    (when-not (get-and-set! has-run-once true)
    (do something))
  • Want to use an atom similarly to a java.util.concurrent.LinkedTransferQueue, for the case of pairing up adds by writers and drainTo y readers:
    Thread 1: (swap! atm conj item1)
    Thread 2: (swap! atm conj item2)
    Thread 3: (let [new-vals (get-and-set! atm [])] 
    (do-something new-vals))

Approach:

  • Extends clojure.lang.IAtom with derefReset and derefSwap with the exact same arities as their original variants, .reset and .swap. The deref-methods returns the old (deref-ed) value of the atom after a successful mutation of the atom.
  • Added clojure.core/deref-reset! - just like reset! it unconditionally changes values of atoms but returns
    the old atom value. Based on Steven Yi's get-and-set! in ticket CLJ-1599, patch named get-and-set.diff
  • Added clojure.core/deref-swap! - just like swap! but returns the old atom value.

Patch: deref-swap-deref-reset-with-IAtomDeref-and-tests.diff



 Comments   
Comment by Philip Potter [ 28/Jun/14 4:00 PM ]

Overtone already defines functions like this in overtone.helpers.ref, which get used by overtone.libs.event. These return both the old and the new value, although in all existing use cases only the old value gets used.

flatland/useful defines a trade! fn which returns the old value, although the implementation is less clean than a compare-and-set! based solution would be.

Comment by Philip Potter [ 29/Jun/14 6:23 AM ]

Chris Ford suggested "swap-out!" as a name for this function. I definitely think "swap-returning-old!" isn't the ideal name.

Comment by Jozef Wagner [ 30/Jun/14 1:33 AM ]

I propose a switch! name. The verb switch is defined as "substitute (two items) for each other; exchange.", and as you get the old value back, it evokes slightly the exchange of items.

Comment by Philip Potter [ 30/Jun/14 3:03 AM ]

Medley also has a deref-swap! which does the same thing.

Comment by Alex Miller [ 30/Jun/14 8:20 AM ]

I think deref-swap! seems like a morally equivalent name to Java's AtomicReference.getAndSet() which is the same idea.

Comment by Philip Potter [ 30/Jun/14 1:19 PM ]

Funny you say that Alex, because prismatic/plumbing defines a get-and-set! (also defined by other projects), equivalent to deref-reset! in medley. Plumbing also defines swap-pair! which returns both old and new values, like the overtone fn, although once again the only usage I can find only uses the old value.

Comment by Alex Miller [ 30/Jun/14 3:37 PM ]

I think it's important to retain the notion that you are not switching/exchanging values but applying the update model of applying a function to the old value to produce the new value. I don't even particularly like "swap!" as I think that aspect is lost in the name (alter and alter-var-root are better). I like that "deref-swap!" combines two words with existing connotations and orders them appropriately.

Comment by Timothy Baldridge [ 30/Jun/14 3:43 PM ]

except that that naming doesn't fit well compared to functions like nfirst which are defined as (comp next first). This function is not (comp deref swap!).

Comment by Linus Ericsson [ 29/Feb/16 11:03 AM ]

The patch deref-swap-deref-reset-extending-IAtom.diff addresses CLJ-1454 and CLJ-1599 "Add deref-reset! (reset! that returns old value)".

The patch extends IAtom with .derefSwap and .derefReset methods with identical signatures as to .swap and .reset but are intended to return the derefed (old) value of the atom upon successful mutation of the atom.

The commit message (could be used as an updated description of this ticket, or a new one)

Extends clojure.lang.IAtom with derefReset and derefSwap with the
exact same arities as their original variants, .reset and .swap.

The deref-methods returns the old (derefed) value of the atom after
a successful mutation of the atom.

Added functions:

clojure.core/deref-reset!
Just like reset! it unconditionally changes values of atoms but returns
the old atom value.

clojure.core/deref-swap!
Just like swap! but returns the old atom value.

The deref-reset is based on Steven Yi's get-and-set! in ticket CLJ-1599,
patch named get-and-set.diff

Comment by Alex Miller [ 01/Mar/16 8:46 AM ]

Review:

  • remove ":static true" in the new fns, this is dead
  • as mentioned on the mailing list, I think we need to consider that we are introducing a breaking change here. Rather than modifying IAtom we should add the methods in a new interface IAtomDeref that Atom implements. The Clojure functions should then type-hint to IAtomDeref instead.
Comment by Alex Miller [ 01/Mar/16 8:47 AM ]

Oh, and needs tests!

Comment by Linus Ericsson [ 02/Mar/16 2:14 PM ]

Alex, in deref-swap-deref-reset-with-IAtomDeref-and-tests.diff you'll find a patch with the differences

  • removed ":static true"
  • added an IAtomDeref interface instead of extending IAtom
  • 2 test cases for deref-reset!
  • 1 test case for deref-swap!

Please especially have a look at the test-cases. I tried to catch the various arities of deref-swap! but I don't know if binding warn-on-reflection true actually affects the test cases in any way. Suggestions on how to test wether "arity works" - if even worth the effort to test - are most welcome.

Given the nature of atoms, a suite of generative tests would probably be the best idea. Until then, simple functional tests for the other atom functions should be added, but that's for another ticket.

Comment by Linus Ericsson [ 02/Mar/16 2:34 PM ]

(the second patch is independent from the first one)

Comment by Alex Miller [ 10/Mar/16 9:13 AM ]

Looks good to me, marking prescreened.





[CLJ-1797] Mention cljc when require fails Created: 10/Aug/15  Updated: 09/Mar/16

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

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

 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-1398] Update URLs in javadoc.clj Created: 02/Apr/14  Updated: 08/Mar/16

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

Type: Defect Priority: Minor
Reporter: Eli Lindsey Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: None

Attachments: Text File 0001-update-apache-commons-javadoc-location.patch     Text File 0002-add-javadoc-lookup-for-guava-and-apache-commons-lang.patch     Text File 0003-add-javadoc-lookup-for-jdk8.patch     Text File clj-1398.patch    
Patch: Code
Approval: Prescreened

 Description   

javadoc.clj contains javadoc base urls for the jdk and several popular open source libraries. Most of these urls are broken.

For example, create a project with the following dependencies:

[org.clojure/clojure "1.8.0"]
[commons-codec "1.10"]
[commons-io "2.4"]
[commons-lang "2.6"]
[org.apache.commons/commons-lang3 "3.4"]
[com.google.guava/guava "19.0"]

And then try the following javadoc checks:

(javadoc java.lang.AutoCloseable)
(javadoc java.util.function.Function)
(javadoc org.apache.commons.codec.Decoder)
(javadoc org.apache.commons.io.IOUtils)
(javadoc org.apache.commons.lang.ArrayUtils)
(javadoc org.apache.commons.lang3.ArrayUtils)
(javadoc com.google.common.collect.BiMap)

Fixes:

  • Update jdk javadoc urls and explicitly include 1.6, 1.7, and 1.8, falling back to 1.8 (most common in use right now)
  • Update commons-codec javadoc url
  • Update commons-io javadoc url
  • Update commons-lang javadoc url (not that commons-lang last release was 2.6)
  • Add commons-lang3 javadoc url (the 3+ version of commons-lang changed the base package name)
  • Add guava javadoc url (another very common Java library)

Patch: clj-1398.patch



 Comments   
Comment by Andy Fingerhut [ 04/Apr/14 11:22 AM ]

Eli, thanks for the patches. It appears that you are not currently on the list of Clojure contributors here: http://clojure.org/contributing

It is the policy of the Clojure team only to incorporate patches submitted by people who have signed and submitted a Clojure CA. Were you interested in doing that?

Comment by Eli Lindsey [ 04/Apr/14 11:27 AM ]

> It is the policy of the Clojure team only to incorporate patches submitted by people who have signed and submitted a Clojure CA. Were you interested in doing that?

Yup! I mailed off the CA to Rich on Wednesday when this was filed; should be arriving shortly.

Comment by Eli Lindsey [ 09/May/14 8:18 PM ]

Just to note - Clojure CA went through and I'm listed on the contributors page now.

Comment by Alex Miller [ 01/Feb/16 6:15 AM ]

I squashed the patches and made some minor modifications in the jdk urls. Patch attribution retained.

Comment by Bozhidar Batsov [ 08/Mar/16 3:15 PM ]

See also https://github.com/clojure-emacs/cider-nrepl/issues/308#issuecomment-193551333

Comment by Alex Miller [ 08/Mar/16 5:27 PM ]

Thanks Bozhidar - I expect this will get updated for Clojure 1.9.

On the note in the change in method urls, the clojure.java.javadoc/javadoc function only builds urls to classes, not to methods, so I don't think that note has any impact on the patch.





[CLJ-1898] Inconsistent duplicate check in set/map literals with quoted/unquoted equal constants Created: 06/Mar/16  Updated: 06/Mar/16

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, compiler

Approval: Triaged

 Description   

Set and map literals containing the same constant quoted and unquoted, will throw a duplicate key exception in some cases (the correct behaviour), while silently ignore the duplicate in some others.

user=> #{'1 1}
#{1}
user=> #{'[] []}
IllegalArgumentException Duplicate key: []  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:56)

This happens because the compiler assumes that literals that have distinct elements at read-time, will have distinct elements at runtime. This is not true for self-evaluating elements where (quote x) is equal to x






[CLJ-445] Method/Constructor resolution does not factor in widening conversion of primitive args Created: 29/Sep/10  Updated: 04/Mar/16

Status: In Progress
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Backlog

Type: Enhancement Priority: Major
Reporter: Alexander Taggart Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File clj-445-prim-conversion-update-2-patch.txt     Text File prim-conversion.patch     Text File prim-conversion-update-1.patch     Text File reorg-reflector.patch    
Approval: Vetted

 Description   

Problem:
When making java calls (or inlined functions), if both args and param are primitive, no widening conversion is used to locate the proper overloaded method/constructor.

Examples:

user=> (Integer. (byte 0))
java.lang.IllegalArgumentException: No matching ctor found for class java.lang.Integer (NO_SOURCE_FILE:0)
</code></pre>
The above occurs because there is no Integer(byte) constructor, though it should match on Integer(int).
<pre><code>user=> (bit-shift-left (byte 1) 1)
Reflection warning, NO_SOURCE_PATH:3 - call to shiftLeft can't be resolved.
2

In the above, a call is made via reflection to Numbers.shiftLeft(Object, Object) and its associated auto-boxing, instead of directly to the perfectly adequate Numbers.shiftLeft(long, int).

Workarounds:
Explicitly casting to the formal type.

Ancillary benefits of fixing:
It would also reduce the amount of method overloading, e.g., RT.intCast(char), intCast(byte), intCast(short), could all be removed, since such calls would pass to RT.intCast(int).



 Comments   
Comment by Assembla Importer [ 23/Oct/10 6:43 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/445
Attachments:
fixbug445.diff - https://www.assembla.com/spaces/clojure/documents/b6gDSUZOur36b9eJe5cbCb/download/b6gDSUZOur36b9eJe5cbCb

Comment by Assembla Importer [ 23/Oct/10 6:43 PM ]

ataggart said: [file:b6gDSUZOur36b9eJe5cbCb]

Comment by Assembla Importer [ 23/Oct/10 6:43 PM ]

ataggart said: Also fixes #446.

Comment by Stuart Halloway [ 03/Dec/10 12:50 PM ]

The patch is causing a test failure

[java] Exception in thread "main" java.lang.IllegalArgumentException: 
     More than one matching method found: equiv, compiling:(clojure/pprint/cl_format.clj:428)

Can you take a look?

Comment by Stuart Halloway [ 28/Jan/11 12:30 PM ]

The failing test happens when trying to find the correct equiv for signature (Number, long). Is the compiler wrong to propose this signature, or is the resolution method wrong in not having an answer? (It thinks two signatures are tied: (Object, long) and (Number, Number).)

Exception in thread "main" java.lang.IllegalArgumentException: More than one matching method found: equiv, compiling:(clojure/pprint/cl_format.clj:428)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6062)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6050)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.access$100(Compiler.java:35)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5492)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2372)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3277)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6057)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2385)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2385)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$FnMethod.parse(Compiler.java:4667)
	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3397)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6053)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.access$100(Compiler.java:35)
	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:480)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler.eval(Compiler.java:6114)
	at clojure.lang.Compiler.load(Compiler.java:6545)
	at clojure.lang.RT.loadResourceScript(RT.java:340)
	at clojure.lang.RT.loadResourceScript(RT.java:331)
	at clojure.lang.RT.load(RT.java:409)
	at clojure.lang.RT.load(RT.java:381)
	at clojure.core$load$fn__1427.invoke(core.clj:5308)
	at clojure.core$load.doInvoke(core.clj:5307)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.pprint$eval3969.invoke(pprint.clj:46)
	at clojure.lang.Compiler.eval(Compiler.java:6110)
	at clojure.lang.Compiler.load(Compiler.java:6545)
	at clojure.lang.RT.loadResourceScript(RT.java:340)
	at clojure.lang.RT.loadResourceScript(RT.java:331)
	at clojure.lang.RT.load(RT.java:409)
	at clojure.lang.RT.load(RT.java:381)
	at clojure.core$load$fn__1427.invoke(core.clj:5308)
	at clojure.core$load.doInvoke(core.clj:5307)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.core$load_one.invoke(core.clj:5132)
	at clojure.core$load_lib.doInvoke(core.clj:5169)
	at clojure.lang.RestFn.applyTo(RestFn.java:143)
	at sun.reflect.GeneratedMethodAccessor11.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:597)
	at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:77)
	at clojure.lang.Reflector.invokeInstanceMethod(Reflector.java:28)
	at clojure.core$apply.invoke(core.clj:602)
	at clojure.core$load_libs.doInvoke(core.clj:5203)
	at clojure.lang.RestFn.applyTo(RestFn.java:138)
	at sun.reflect.GeneratedMethodAccessor11.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:597)
	at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:77)
	at clojure.lang.Reflector.invokeInstanceMethod(Reflector.java:28)
	at clojure.core$apply.invoke(core.clj:604)
	at clojure.core$use.doInvoke(core.clj:5283)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.main$repl.doInvoke(main.clj:196)
	at clojure.lang.RestFn.invoke(RestFn.java:422)
	at clojure.main$repl_opt.invoke(main.clj:267)
	at clojure.main$main.doInvoke(main.clj:362)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.lang.Var.invoke(Var.java:401)
	at clojure.lang.AFn.applyToHelper(AFn.java:163)
	at clojure.lang.Var.applyTo(Var.java:518)
	at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: More than one matching method found: equiv
	at clojure.lang.Reflector.getMatchingParams(Reflector.java:639)
	at clojure.lang.Reflector.getMatchingParams(Reflector.java:578)
	at clojure.lang.Reflector.getMatchingMethod(Reflector.java:569)
	at clojure.lang.Compiler$StaticMethodExpr.<init>(Compiler.java:1439)
	at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:896)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	... 115 more
Comment by Alexander Taggart [ 08/Feb/11 6:27 PM ]

In working on implementing support for vararg methods, I found a number of flaws with the previous solutions. Please disregard them.

I've attached a single patch (reflector-compiler-numbers.diff) which is a rather substantial overhaul of the Reflector code, with some enhancements to the Compiler and Numbers code.

The patch notes:

  • Moved reflection functionality from Compiler to Reflector.
  • Reflector supports finding overloaded methods by widening conversion, boxing conversion, and casting.
  • During compilation Reflector attempts to find best wildcard match.
  • Reflector refers to *unchecked-math* when reflectively invoking methods and constructors.
  • Both Reflector and Compiler support variable arity java methods and constructor; backwards compatible with passing an array or nil in the vararg slot.
  • Added more informative error messages to Reflector.
  • Added tests to clojure.test-clojure.reflector.
  • Altered overloaded functions of clojure.lang.Numbers to service Object/double/long params; fixes some ambiguity issues and avoids unnecessary boxing in some cases.
  • Patch closes issues 380, 440, 445, 666, and possibly 259 (not enough detail provided).
Comment by Alexander Taggart [ 10/Feb/11 7:35 PM ]

Updated patch to fix a bug where a concrete class with multiple identical Methods (e.g., one from an interface, another from an abstract class) would result in ambiguity. Now resolved by arbitrary selection (this is what the original code did as well albeit not explicitly).

Comment by Alexander Taggart [ 25/Feb/11 9:29 PM ]

Updated patch to work with latest master branch.

Comment by Stuart Halloway [ 06/Mar/11 1:54 PM ]

patch appears to be missing test file clojure/test_clojure/reflector.clj.

Comment by Alexander Taggart [ 06/Mar/11 2:39 PM ]

Bit by git.

Patch corrected to contain clojure.test-clojure.reflector.

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

Rich: I verified that the patch applied but reviewed only briefly, knowing you will want to go over this one closely.

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

After applying this patch, I am getting method missing errors:

java.lang.NoSuchMethodError: clojure.lang.Numbers.lt(JLjava/lang/Object;)

but only when using compiled code, e.g. the same code works in the REPL and then fails after compilation. Haven't been able to isolate an example that I can share here yet, but hoping this will cause someone to have an "a, hah" moment...

Comment by Alexander Taggart [ 02/Apr/11 12:55 PM ]

The patch now contains only the minimal changes needed to support widening conversion. Cleanup of Numbers overloads, etc., can wait until this patch gets applied. The vararg support is in a separate patch on CLJ-440.

Comment by Christopher Redinger [ 15/Apr/11 12:50 PM ]

Please test patch

Comment by Christopher Redinger [ 21/Apr/11 11:02 AM ]

FYI: Patch applies cleanly on master and all tests pass as of 4/21 (2011)

Comment by Christopher Redinger [ 22/Apr/11 9:57 AM ]

This work is too big to take into the 1.3 beta right now. We'll revisit for a future release.

Comment by Alexander Taggart [ 28/Apr/11 1:19 PM ]

To better facilitate understanding of the changes, I've broken them up into two patches, each with a number of isolable, incremental commits:

reorg-reflector.patch: Moves the reflection/invocation code from Compiler to Reflector, and eliminates redundant code. The result is a single code base for resolving methods/constructors, which will allow for altering that mechanism without excess external coordination. This contains no behaviour changes.

prim-conversion.patch: Depends on the above. Alters the method/constructor resolution process:

  • more consistent with java resolution, especially when calling pre-1.5 APIs
  • adds support for widening conversion of primitive numerics of the same category (this is more strict than java, and more clojuresque)
  • adds support for wildcard matches at compile-time (i.e., you don't need to type-hint every arg to avoid reflection).

This also provides a base to add further features, e.g., CLJ-666.

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

It's documented in situ, but here are the conversion rules that the reflector uses to find methods:

  1. By Type:
    • object to ancestor type
    • primitive to a wider primitive of the same numeric category (new)
  2. Boxing:
    • boxed number to its primitive
    • boxed number to a wider primitive of the same numeric category (new for Short and Byte args)
    • primitive to its boxed value
    • primitive to Number or Object (new)
  3. Casting:
    • long to int
    • double to float
Comment by Alexander Taggart [ 10/May/11 3:13 PM ]

prim-conversion-update-1.patch applies to current master.

Comment by Alexander Taggart [ 11/May/11 2:15 PM ]

Created CLJ-792 for the reflector reorg.

Comment by Stuart Sierra [ 17/Feb/12 2:29 PM ]

prim-conversion-update-1.patch does not apply as of f5bcf64.

Is CLJ-792 now a prerequisite of this ticket?

Comment by Alexander Taggart [ 17/Feb/12 3:15 PM ]

Yes, after the original patch was deemed "too big".

After this much time with no action from TPTB, feel free to kill both tickets.

Comment by Andy Fingerhut [ 20/Feb/12 2:04 PM ]

Again, not sure if this is any help, but I've tested starting from Clojure head as of Feb 20, 2012, applying clj-792-reorg-reflector-patch2.txt attached to CLJ-792, and then applying clj-445-prim-conversion-update-2-patch.txt attached to this ticket, and the result compiles and passes all but 2 tests. I don't know whether those failures are easy to fix or not, or whether issues may have been introduced with these patches.

Comment by Nicola Mometto [ 30/Jan/15 7:25 AM ]

As of 1.7.0-alpha5 the two examples in the ticket description run without exceptions/reflection-warnings

user=> (set! *warn-on-reflection* true)
true
user=> (bit-shift-left (byte 1) 1)
2
user=> (Integer. (byte 0))
0
Comment by Petr Yanovich [ 04/Mar/16 7:17 AM ]

Hi!

(Integer. (Long. 1))
#=> 1
(Long. (Integer. 1))
#=> Exception

https://github.com/clojure/clojure/pull/59





[CLJ-1860] 0.0 and -0.0 compare equal but have different hash values Created: 01/Dec/15  Updated: 29/Feb/16

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

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

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

 Description   

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

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

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

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

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

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

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

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

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

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

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

Patch: CLJ-1860-negative-zero-hash-eq-fix.patch

Prescreened: Alex Miller

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Is this acceptable?

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

return new Double(-0.0).hashCode()

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

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

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

Same for the float case, of course.

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

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

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

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

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

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

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

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

Thanks!

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

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

Reasons:

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

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

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

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

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

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

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

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

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

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

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





[CLJ-1862] Release both a direct linked and a non direct linked clojure Created: 02/Dec/15  Updated: 25/Feb/16

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: release


 Description   

Currently all new clojure releases will have the core library direct linked.
We should distribute both a direct linked and non direct linked alternatives, using a different classifier for the release.






[CLJ-1890] enhance pprint to print type for defrecord (as in pr) Created: 05/Feb/16  Updated: 25/Feb/16

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

Type: Enhancement Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: pprint, print

Attachments: Text File CLJ-1890-pprint-records.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

pprint currently doesn't print the names of defrecord types, instead printing just the underlying map. This is in contrast to pr-str/println. This ticket proposes that the behaviour of pprint is changed to match pr-str and println's.

More discussion at https://groups.google.com/forum/#!topic/clojure-dev/lRDG6a5eE-s

user=> (defrecord myrec [a b])
user.myrec
user=> (->myrec 1 2)
#user.myrec{:a 1, :b 2}
user=> (pr-str (->myrec 1 2))
"#user.myrec{:a 1, :b 2}"
user=> (println (->myrec 1 2))
#user.myrec{:a 1, :b 2}
nil
user=> (pprint (->myrec 1 2))
{:a 1, :b 2}
nil

Approach: Add new IRecord case to pprint's simple-dispatch mode. Extract guts of pprint-map to pprint-map-kvs and call it from both existing pprint-map and new pprint-record. Set multimethod preference for IRecord version. Added test.

user=> (pprint (->myrec 1 2))
#user.myrec{:a 1, :b 2}

Patch: CLJ-1890-pprint-records.patch

Prescreened by: Alex Miller



 Comments   
Comment by Daniel Compton [ 05/Feb/16 1:51 PM ]

The fix for this will needed to be ported to ClojureScript too.

Comment by Steve Miner [ 06/Feb/16 11:55 AM ]

Added patch to pprint records with classname.

Comment by Steve Miner [ 06/Feb/16 12:03 PM ]

Open question: How should pprint handle a record that has a print-method defined for it? Should the print-method be used instead of the pprint default?

The current release and my patch do not consider the print-method when calling pprint.

Comment by Alex Miller [ 08/Feb/16 4:33 PM ]

I do not think pprint should check for or use print-method. pprint has it's own simple-dispatch multimethod that you can extend, or it will ultimately fall through to pr (which can be extended via print-dup).

Example of the former:

user=> (defrecord R [a])
user=> (def r (->R 1))
user=> (pprint r)
{:a 1}

user=> (use 'clojure.pprint)
user=> (defmethod simple-dispatch user.R [r] (pr r))
#object[clojure.lang.MultiFn 0x497470ed "clojure.lang.MultiFn@497470ed"]
user=> (pprint r)
#user.R{:a 1}
Comment by Steve Miner [ 09/Feb/16 6:27 AM ]

Right, clojure.pprint/simple-dispatch is there for user code to customize pprint, independent of whatever they might do with print-method. No need to conflate the two. So the patch is ready to review.





[CLJ-1832] unchecked-* functions have different behavior on primitive longs vs boxed Longs Created: 26/Oct/15  Updated: 24/Feb/16

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

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

 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

Prescreened: 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-1896] Support transducers in vec and set fns Created: 24/Feb/16  Updated: 24/Feb/16

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

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

Approval: Triaged

 Description   

Rather than

(into [] (map inc) [1 2 3])
vec (and set) could support the transducer directly:

(vec (map inc) [1 2 3])
(set (map inc) #{1 2 3})

Depending how far we wanted to take this, the implementation could be somewhat clever for vec in building the initial set of results in an array and then creating the vector with it directly as is already done in some other cases.






[CLJ-1895] Remove loading of clojure.string in clojure.java.io Created: 22/Feb/16  Updated: 22/Feb/16

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

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

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

 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-1874] Var redefinition breaks in AOT-compiled code Created: 05/Jan/16  Updated: 18/Feb/16

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

Type: Defect Priority: Major
Reporter: William Parker Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: aot, compiler

Attachments: Text File 0001-CLJ-1874-ensure-vars-get-interned-when-AOT-compiled.patch    
Patch: Code

 Description   

This is basically a copy of my post from https://groups.google.com/forum/#!topic/clojure/Ozt5HQyM36I on the mailing list. Based on the replies there I'm not sure whether this should be logged as an enhancement or a defect. Please change the designation to whatever is appropriate.

I have found what appears to be a bug in AOT-compiled Clojure when ns-unmap is used; the root cause of this probably impacts other code that redefines Vars as well. I have the following reduced case:

(ns unmap-test.core)

(def a :test-1)

(ns-unmap 'unmap-test.core 'a)

(def a :test-2)

It turns out that a is not resolvable when this namespace is loaded. When I looked at the compiled bytecode,
it appears that the following operations occur:

1. A call to RT.var withe 'unmap-test.core and 'a returns a Var, which is bound to a constant.
This var is added to the namespaces's mapping during this call.
2. Same as 1.
3. The var from 1 is bound to :test-1.
4. ns-unmap is called.
5. The var from 2 is bound to :test-2.

Disclaimer: This is the first time I have had occasion to look directly at bytecode and I could be missing something.

The basic problem here is that the var is accessible from the load method, but when step 5 executes the var is no longer
accessible from the namespace mappings. Thus, the root of the Var is set to :test-2, but that Var is not mapped from the namespace.
This works when there is no AOT compilation, as well as when I use

(ns unmap-test.core)

(def a :test-1)

(ns-unmap 'unmap-test.core 'a)

(intern 'unmap-test.core 'a :test-2)

I realize that creating defs, unmapping them, and then recreating them is generally poor practice in Clojure.
We have an odd case in that we need to have an interface and a Var of the same name in the same namespace. Simply
doing definterface and then def causes a compilation failure:

user=> (definterface abc)
user.abc
user=> (def abc 1)
CompilerException java.lang.RuntimeException: Expecting var, but abc is mapped to interface user.abc, compiling/private/var/folders/3m/tvc28b5d7p50v5_8q5ntj0pmflbdh9/T/form-init4734176956271823921.clj:1:1)

Without going into too much detail, this is basically a hack to allow us to refactor our internal framework code
without immediately changing a very large amount of downstream consumer code. We get rid of the usage of the interface during macroexpansion,
but it still needs to exist on the classpath so it can be imported by downstream namespaces.
There are a number of other ways to accomplish this, so it isn't a particularly big problem for us, but I thought the issue was worth raising.
This was just the first thing I tried and I was surprised when it didn't work.

Note that I used the 1.8.0 RC4 version of Clojure in my sample project, but I had the same behavior on 1.7.0.

Relevant links:

1. Bytecode for the load method of the init class: https://gist.github.com/WilliamParker/d8ef4c0555a30135f35a
2. Bytecode for the init0 method: https://gist.github.com/WilliamParker/dc606ad086670915efd9
3. Decompiled Java code for the init class. Note that this does not completely line up with the bytecode as far as I can tell,
but it is a quicker way to get a general idea of what is happening than the bytecode.
https://gist.github.com/WilliamParker/4cc47939f613d4595d94
4. A simple project containing the code above: https://github.com/WilliamParker/unmap-test
Note that if you try it without AOT compilation the target folder with any previously compiled classes should be removed.



 Comments   
Comment by Nicola Mometto [ 05/Jan/16 9:44 AM ]

The issue is similar to the one in CLJ-1604, the proposed patch extends that fix to all vars rather than just for clojure.core ones.

Comment by Kevin Downey [ 17/Feb/16 10:21 AM ]

was this fixed by clj-1604?

Comment by Nicola Mometto [ 18/Feb/16 8:38 AM ]

No, CLJ-1604 only deals with clojure.core Vars, the patch attached to this ticket is an extension on top of the patch committed for CLJ-1604 that deals with other namespaces





[CLJ-1206] 'eval' of closures or fns with runtime metadata within a call expr yields "No matching ctor found" exceptions Created: 28/Apr/13  Updated: 18/Feb/16

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

Type: Defect Priority: Minor
Reporter: Jason Wolfe Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None


 Description   

I ran into some issues with 'eval' when writing compilation strategies for Graph. It seems these may have been known for some time [1], but I couldn't find a ticket for them, so here we are.

Clojure docs [2] say "If the operator is not a special form or macro, the call is considered a function call. Both the operator and the operands (if any) are evaluated, from left to right," and "Any object other than those discussed above will evaluate to itself." While bare fns do seem to evaluate to themselves in all cases, when in a call expression, the evaluation of the operator fails on fn objects that are closures or have run-time metadata applied:

;; raw non-closures are fine
user> (eval (fn [x] (inc x)))
#<user$eval30559$fn_30560 user$eval30559$fn_30560@354ee11c>

;; raw closures are fine
user> (eval (let [y 1] (fn [x] (+ x y))))
#<user$eval30511$fn_30512 user$eval30511$fn_30512@3bac3a34>

;; non-closures in exprs are fine
user> (eval `(~(fn [x] (inc x)) 1))
2

;; but closures in exprs cause an error
user> (eval `(~(let [y 1] (fn [x] (+ x y))) 1))
IllegalArgumentException No matching ctor found for class user$eval30535$fn__30536 clojure.lang.Reflector.invokeConstructor (Reflector.java:163)

;; as do fns with metadata in exprs
user> (eval `(~(with-meta (fn [x] (inc x)) {:x 1}) 1))
IllegalArgumentException No matching ctor found for class clojure.lang.AFunction$1 clojure.lang.Reflector.invokeConstructor (Reflector.java:163)

[1] http://stackoverflow.com/a/11287181
[2] http://clojure.org/evaluation



 Comments   
Comment by Dmitri Gaskin [ 17/Feb/16 9:25 PM ]

I have run into this as well. I ran into it while using https://github.com/plumatic/schema + a memoized predicate function + https://github.com/metosin/compojure-api. After hours of debugging, I was able to narrow it down to:

```
user=> (let [foo inc] (eval `(~foo 1)))
2
user=> (let [foo (memoize inc)] (eval `(~foo 1)))

IllegalArgumentException No matching ctor found for class clojure.core$memoize$fn__5708 clojure.lang.Reflector.invokeConstructor (Reflector.java:163)
```

Comment by Kevin Downey [ 17/Feb/16 11:01 PM ]

basically, this cannot work. as it stands now, for some set of fn objects (including closures) eval fails, because the compiler cannot embed arbitrary function objects in the emitted bytecode. for another set of fn objects eval succeeds, because there is a fall back in eval, where it tries to embed arbitrary objects in the byte code by invoking a no argument constructor on the class of the object. closures fail to work in that fall back because the classes generated for fns that are closures in clojure don't have 0-arg constructors.

don't eval function objects

Comment by Kevin Downey [ 17/Feb/16 11:20 PM ]

for a work around maybe try using dynamic binding?

Comment by Jason Wolfe [ 18/Feb/16 12:35 AM ]

Here is a workaround that we use in graph: https://github.com/plumatic/plumbing/blob/master/src/plumbing/graph/positional.clj#L45

Basically, rather than evaluating a form with function objects inside, e.g. `(eval (... f))`, we do: `((eval (fn [x] (... x))) f)`.





[CLJ-1876] calling require from java is not thread safe Created: 07/Jan/16  Updated: 17/Feb/16

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

Type: Defect Priority: Major
Reporter: Robert (Bobby) Evans Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Crappy Linux VM running RHEL6

java version "1.8.0_60"
Java(TM) SE Runtime Environment (build 1.8.0_60-b27)
Java HotSpot(TM) 64-Bit Server VM (build 25.60-b23, mixed mode)



 Description   

As a part of Apache Storm we have some code that can load a clojure function from java using the following code.

public static IFn loadClojureFn(String namespace, String name) {
        try {
            clojure.lang.Compiler.eval(RT.readString("(require '" + namespace + ")"));
        } catch (Exception e) {
            //if playing from the repl and defining functions, file won't exist
        }
        return (IFn) RT.var(namespace, name).deref();
    }

If this function is called from multiple different threads at the same time, trying to import the same namespace, I will occasionally get some very odd errors. NOTE: I had to modify the catch to actually print out the error message it was getting (We should not be eating exceptions either way).

{verbatim}
2016-01-07 16:26:09.305 b.s.u.Utils [WARN] Loading namespace failed
clojure.lang.Compiler$CompilerException: java.lang.RuntimeException: Unable to resolve symbol: sentence-spout in this context, compiling:(storm/starter/clj/word_count.clj:21:1)
at clojure.lang.Compiler.analyze(Compiler.java:6543) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6485) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3791) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6725) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6524) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6485) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.eval(Compiler.java:6786) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.load(Compiler.java:7227) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.loadResourceScript(RT.java:371) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.loadResourceScript(RT.java:362) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.load(RT.java:446) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.load(RT.java:412) ~[clojure-1.7.0.jar:?]
at clojure.core$load$fn__5448.invoke(core.clj:5866) ~[clojure-1.7.0.jar:?]
at clojure.core$load.doInvoke(core.clj:5865) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.invoke(RestFn.java:408) ~[clojure-1.7.0.jar:?]
at clojure.core$load_one.invoke(core.clj:5671) ~[clojure-1.7.0.jar:?]
at clojure.core$load_lib$fn__5397.invoke(core.clj:5711) ~[clojure-1.7.0.jar:?]
at clojure.core$load_lib.doInvoke(core.clj:5710) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.applyTo(RestFn.java:142) ~[clojure-1.7.0.jar:?]
at clojure.core$apply.invoke(core.clj:632) ~[clojure-1.7.0.jar:?]
at clojure.core$load_libs.doInvoke(core.clj:5749) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.applyTo(RestFn.java:137) ~[clojure-1.7.0.jar:?]
at clojure.core$apply.invoke(core.clj:632) ~[clojure-1.7.0.jar:?]
at clojure.core$require.doInvoke(core.clj:5832) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.invoke(RestFn.java:408) ~[clojure-1.7.0.jar:?]
at clojure.core$eval114.invoke(NO_SOURCE_FILE:0) ~[?:?]
at clojure.lang.Compiler.eval(Compiler.java:6782) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.eval(Compiler.java:6745) ~[clojure-1.7.0.jar:?]
at backtype.storm.utils.Utils.loadClojureFn(Utils.java:602) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.clojure.ClojureBolt.prepare(ClojureBolt.java:57) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.daemon.executor$fn_8297$fn_8310.invoke(executor.clj:785) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.util$async_loop$fn__556.invoke(util.clj:482) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at clojure.lang.AFn.run(AFn.java:22) [clojure-1.7.0.jar:?]
at java.lang.Thread.run(Thread.java:745) [?:1.8.0_60]
Caused by: java.lang.RuntimeException: Unable to resolve symbol: sentence-spout in this context
at clojure.lang.Util.runtimeException(Util.java:221) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.resolveIn(Compiler.java:7019) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.resolve(Compiler.java:6963) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyzeSymbol(Compiler.java:6924) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6506) ~[clojure-1.7.0.jar:?]
... 33 more{verbatim}

If I make the static java function synchronized the issue goes away. It always seems to blow up when parsing a few specific macros getting confused that a specific symbol cannot be resolved.

The namespace trying to be loaded.
https://github.com/apache/storm/blob/a99d9c11be005ade7c308bebdda71c7fb0111acc/examples/storm-starter/src/clj/storm/starter/clj/word_count.clj

The macros that we seem to get exceptions on.
https://github.com/apache/storm/blob/a99d9c11be005ade7c308bebdda71c7fb0111acc/storm-core/src/clj/backtype/storm/clojure.clj#L77-L138

And like I said it look like it is a threading issue of some sort. When I added the synchronized keyword everything works.



 Comments   
Comment by Kevin Downey [ 17/Feb/16 10:19 AM ]

calling require from clojure isn't thread safe either, no different from this





[CLJ-1317] clojure.zip/seq-zip returns spurious nils during traversal Created: 31/Dec/13  Updated: 10/Feb/16

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

Type: Defect Priority: Minor
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Unresolved Votes: 1
Labels: zip

Attachments: Text File 0001-CLJ-1317-fix-seq-zip-to-avoid-spurious-nils.patch    
Patch: Code
Approval: Triaged

 Description   

Problem reported by Lee Spector on the mailing list:

https://groups.google.com/d/msg/clojure/8TL7IGmE7N0/u1xfgTOLDRgJ

Here's a quote from Lee's post describing the problem:

Here's an illustration, stepping through '(() 0) with next and printing the node at each step: 

(loop [z (zip/seq-zip '(() 0))] 
  (if (zip/end? z) 
    :done 
    (do (println (zip/node z)) 
      (recur (zip/next z))))) 

That produces: 

(() 0) 
() 
nil 
0 
:done 

I don't expect the nil to be there. 

The underlying cause is that seq-zip passes identity as the children argument to zipper. Applied to (), this returns (), which is truthy, leading zipper to descend into a non-existent subtree.

One natural solution would be to use seq in place of identity:

(defn seq-zip [root]
  (zipper seq?
          seq  ;; changed
          (fn [node children] (with-meta children (meta node)))
          root))

With this change, no nil is produced in the example above. Patch with this change forthcoming.



 Comments   
Comment by Michał Marczyk [ 31/Dec/13 5:52 PM ]

Note that the docstring of clojure.zip/zipper asks that the children argument return a seq of children. The rest of clojure.zip, however, expects nil to be returned when there are no children, as evidenced by this problem.

One could argue that this behaviour of the rest of clojure.zip should be fixed, but I think it makes sense and is convenient. Perhaps the docstring should be adjusted, though.

Comment by Alex Miller [ 08/Feb/16 4:36 PM ]

Michał, can I ask why you assigned this to yourself - was there something you planned to add?

Comment by Michał Marczyk [ 10/Feb/16 9:09 AM ]

Hey Alex, I was going to attach a separate patch with a proposal for a docstring adjustment along the lines suggested above (will do that tonight). No change to the code, though, and I guess not worth assigning the ticket – sorry about the unnecessary ping.

Comment by Alex Miller [ 10/Feb/16 9:38 AM ]

No worries, just wanted to know if something was still pending - I will wait to prescreen it.





[CLJ-1857] clojure.string/split docstring does not match the behavior of parameter "limit" Created: 27/Nov/15  Updated: 09/Feb/16

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

Type: Enhancement Priority: Trivial
Reporter: Miikka Koskinen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: string

Attachments: File CLJ_1857_split_docs_update.diff    

 Description   
clojure.string/split
([s re] [s re limit])
  Splits string on a regular expression.  Optional argument limit is
  the maximum number of splits. Not lazy. Returns vector of the splits.

What happens is that limit is the maximum number of parts returned, not the number of splits done. If limit is 1, no splits are done, while I'd expect at most one split to be done. It's a bit of a matter of terminology, but I think that the text could be clarified. Based on ClojureDocs examples, I'm not the only one who was confused.

user=> (str/split "1 2 3" #" ")
["1" "2" "3"]
user=> (str/split "1 2 3" #" " 1)
["1 2 3"]
user=> (str/split "1 2 3" #" " 2)
["1" "2 3"]


 Comments   
Comment by Alex Miller [ 29/Nov/15 2:52 PM ]

To me, the last sentence indicates that "split" (as a noun) is being used to refer to the parts resulting from splitting but there is some ambiguity in the prior sentence. Would "parts" be better?

I don't get your point on the clojuredocs examples - those make sense to me.

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

The docs are a bit ambiguous as "splits" is a verb in the first sentence, but a noun in the other two occurrences. I believe the ClojureDocs example being referred to is likely this one (mostly because of the comment in it):

; Note that the 'limit' arg is the maximum number of strings to
; return (not the number of splits)
user=> (str/split "q1w2e3r4t5y6u7i8o9p0" #"\d+" 5)
["q" "w" "e" "r" "t5y6u7i8o9p0"]

Because split is the name of the function and the action being performed, I think it makes sense to leave it as the verb in the first sentence and replace the other two occurrences with "parts". Does that sound reasonable?





[CLJ-1891] New socket server startup proactively loads too much code, slowing boot time Created: 09/Feb/16  Updated: 09/Feb/16

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: server

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

 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






[CLJ-440] java method calls cannot omit varargs Created: 27/Sep/10  Updated: 03/Feb/16

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

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

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

 Description   

Problem

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

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

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

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

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

Latest patch: Removed because incomplete and goal not clear

Varargs in Java

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

Given the following setup:

VarArgs.java
public class VarArgs {

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

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


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

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

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

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

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

Patch updated to current CLJ-445 patch.

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

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

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

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

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

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

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

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

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

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

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

The patch takes the following approach:

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

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

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

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

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

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

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





[CLJ-1764] partition-by runs infinite loop when one element of infinite partition is accessed Created: 19/Jun/15  Updated: 01/Feb/16

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

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

 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-1077] thread-bound? returns true (implying set! should succeed) even for non-binding thread Created: 26/Sep/12  Updated: 01/Feb/16

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

Type: Defect Priority: Major
Reporter: Paul Stadig Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: None

Attachments: File thread-bound.diff    
Patch: Code
Approval: Prescreened

 Description   

thread-bound? returns true for thread-local bindings that were not bound by the current thread. This is at odds with the docstring, which states that if true is returned, set! should succeed.

ere is an example REPL session where a thread establishes a binding, those bindings are conveyed to a second thread, the second thread checks thread-bound? to see if it can set the binding, thread-bound? returns true indicating that the binding can be set, the second thread tries to set the binding, and the second thread gets an IllegalStateException:

    Clojure 1.5.1
    user=> (def ^:dynamic *set-me* nil)
    #'user/*set-me*
    user=> (defn try-to-set [] (binding [*set-me* 1] (doall (pcalls #(if (thread-bound? #'*set-me*) (set! *set-me* (inc *set-me*)))))))
    #'user/try-to-set
    user=> (try-to-set)
    IllegalStateException Can't set!: *set-me* from non-binding thread  clojure.lang.Var.set (Var.java:230)
    user=> 

Approach: thread-bound? should return false in the case where there is a binding and that binding was not established by the current thread. The patch adds a new function to Var as core does not have visibility into the non-public Var$TBox.

Patch: thread-bound.diff

Screened by: Alex Miller



 Comments   
Comment by Paul Stadig [ 01/Oct/12 10:07 AM ]

I have attached a patch that changes clojure.lang.Var and clojure.core/thread-bound? to only return true if a Var is set!-able.

Comment by Alex Miller [ 19/Aug/13 12:16 PM ]

REPL example?

Comment by Joe Gallo [ 20/Aug/13 7:55 AM ]

Sure thing, Alex – here's a repl example I just ran this morning.

; nREPL 0.1.7
user> (def ^:dynamic *set-me* nil)
#'user/*set-me*
user> (defn try-to-set [] (binding [*set-me* 1] (doall (pcalls #(if (thread-bound? #'*set-me*) (set! *set-me* (inc *set-me*)))))))
#'user/try-to-set
user> (try-to-set)
IllegalStateException Can't set!: *set-me* from non-binding thread  clojure.lang.Var.set (Var.java:230)
user>




[CLJ-1458] Enhance the performance of map merges Created: 04/Jul/14  Updated: 01/Feb/16

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

Type: Enhancement Priority: Critical
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: performance

Attachments: Text File 0001-very-simple-test-of-the-merge-function.patch     Text File clj-1458-4.patch     Text File CLJ-1458-5.patch     Text File CLJ-1458-6.patch     Text File CLJ-1458-transient-merge2.patch     Text File CLJ-1458-transient-merge3.patch     Text File CLJ-1458-transient-merge.patch     Text File merge-test-2.patch     File transient-merge.diff    
Patch: Code and Test
Approval: Triaged

 Description   

It would be nice if merge used transients.

Patch

  • clj-1458-6.patch

Approach
Migrate c.c/merge later in core after transients & reduce. Leave older version as merge1 for use in cases the precede the newer definition. Make APersistentMap/conj & ATransientMap/cons aware of IKVReduce.

The attached patch preserves two existing behaviors of merge

  • metadata propagation
  • the right hand side of the merges can be a Map.Entry, an IPersistentVector where size=2, and regular maps.

Screened by:



 Comments   
Comment by Jason Wolfe [ 13/Sep/14 5:09 PM ]

I will take a crack at a patch today.

Comment by Jason Wolfe [ 13/Sep/14 5:42 PM ]

This patch (transient-merge.diff) makes merge, merge-with, and zipmap (since it was right there and could obviously benefit from transients as well) use transients.

Three potential issues:

  • I had to move the functions, since they depend on transient and friends. I assume this is preferable to a forward declaration. This was the best place I could find, but happy to move them elsewhere.
  • I added multiple arities, to avoid potential performance cost of transient-ing a single argument. Happy to undo this if desired.
  • I had to slightly alter the logic in merge-with, since transient maps don't support contains? (or find).
Comment by Michał Marczyk [ 14/Sep/14 12:43 PM ]

I posted a separate ticket for zipmap, with patch, on 30/May/12: CLJ-1005.

Comment by Jason Wolfe [ 14/Sep/14 5:28 PM ]

Ah, sorry if I overstepped then. Happy to remove that change from this patch then if that will simplify things – just let me know.

Comment by Ghadi Shayban [ 28/Dec/14 10:07 PM ]

alternate approach attached delaying merge until after protocols load, and then using transducers.

Comment by Michael Blume [ 28/Dec/14 11:50 PM ]

Looks like you're doing (get m k) twice – shouldn't that be thrown in a local?

Comment by Michael Blume [ 29/Dec/14 1:41 PM ]

um, put, in a local, I mean, 'throw' was a bad choice of word.

Comment by Ghadi Shayban [ 29/Dec/14 2:14 PM ]

Yeah there's that – won't be using get anyways after CLJ-700 gets committed.

We should add performance tests too. merging two maps, three, many maps, also varying the sizes of the maps, and for merge-with, varying the % of collisions.

Need to go back to the (some identity) logic, otherwise metadata is propagated from maps other than the first provided. I'll fix later.

Comment by Michael Blume [ 29/Dec/14 2:49 PM ]

I don't know if this is supposed to be allowed, but this breaks

(merge {} [:foo 'bar])

which is used in the wild by compojure-api

Comment by Michael Blume [ 29/Dec/14 2:49 PM ]

https://github.com/metosin/compojure-api/blob/0.16.6/src/compojure/api/meta.clj#L198

Comment by Michael Blume [ 29/Dec/14 2:54 PM ]

Ghadi, contains? uses get under the covers, so it's still two gets, right? It seems like it'd be more performant to stick with the ::none trick.

Comment by Nicola Mometto [ 29/Dec/14 5:36 PM ]

This calls for if-let + find.

Comment by Ghadi Shayban [ 29/Dec/14 10:37 PM ]

new patch addressing concerns so far

Comment by Ghadi Shayban [ 29/Dec/14 10:48 PM ]

CLJ-1458-transient-merge3.patch removes silly inlining macro, uses singleton fns instead.

Comment by Michael Blume [ 29/Dec/14 11:14 PM ]

Nice =)

This should come with tests. If we want to preserve the ability to merge with a MapEntry, we should test it. This isn't so much a weakness of the patch as of the existing tests. I see merge and merge-with being used a few times in the test suite, but I see no test whose purpose is to test their behavior.

Comment by Michael Blume [ 29/Dec/14 11:17 PM ]

Extremely simple merge test, we need more than this, but this is a start

Comment by Alex Miller [ 22/Jun/15 10:11 AM ]

clj-1458-4.patch refreshed to apply to master, no changes.

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

I'd like to reevaluate the scope of this ticket. Can we address 'merge' only and defer 'merge-with'? It's by far the more common function. I've attached a new simplified patch.

Comment by Ghadi Shayban [ 09/Jan/16 9:50 PM ]

CLJ-1458-6.patch is yet another cleaner approach

Comment by Alex Miller [ 01/Feb/16 5:17 AM ]

Can you update the ticket approach section to discuss the APersistentMap.cons / ASSOC changes. Also, can you add a before / after perf test for one or more common cases?





[CLJ-1889] Add optional predicate to string trim functions that determines if a character should be trimmed Created: 27/Jan/16  Updated: 28/Jan/16

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

Type: Enhancement Priority: Minor
Reporter: Tamas Szabo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: string

Attachments: Text File CLJ-1889-trim-enhancement.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The proposal is that the trim functions (trim, triml, and trimr) would get a second arity with a function trim?:

[trim? ^CharSequence s]

trim? comes first to support partial.

New doc string would be:

"Removes characters from both ends of string. 
 If trim? is omitted white space is removed. When supplied it accepts 
 a character and returns true if the character should be removed."

Example test:

(deftest t-trim 
  (is (= "foo" (s/trim "  foo  \r\n"))) 
  (is (= "bar" (s/trim "\u2000bar\t \u2002"))) 

  ;; Additional test 
  (is (= "bar" (s/trim "$%#\u2000bar\t \u2002%$#" 
                       #(or (Character/isWhitespace %) ((set "$#%") %))))))

Similar to Python's strip - https://docs.python.org/2/library/stdtypes.html#str.strip

Approach: The proposed solution isn't very DRY but it follows the design guidelines at the top of the file, more exactly point 3:

"3. Functions take advantage of String implementation details to
write high-performing loop/recurs instead of using higher-order
functions. (This is not idiomatic in general-purpose application
code.)"

First I had a solution in which I replaced Character/isWhitespace from the current implementation by calling pred. pred was defaulted to an is-whitespace? function.
That code is of course nicer, even trim-newline could just call into trimr, removing a lot of duplication, but it adds the overhead of always calling a function, instead of calling Character/isWhitespace directly.

The only way I can see to have optimised and DRYer code is to use macros, but I don't think that it will necessary lead to nicer code.

Given the existing design style of the other functions in string.clj I felt that the best solution would be to just simply duplicate in favour of optimised code.



 Comments   
Comment by Tamas Szabo [ 27/Jan/16 2:44 PM ]

Proposed solution. Code + tests.

Comment by Tamas Szabo [ 27/Jan/16 3:42 PM ]

Added new patch that renames pred to trim?

Comment by Andy Fingerhut [ 27/Jan/16 6:27 PM ]

Note that Java, and thus Clojure/Java, uses UTF-16 encoding for strings in memory. Thus if you wanted to trim a set of Unicode code points from the beginning and/or end of a string, the API of trim? taking a single 16-bit Java character is not enough information to determine whether it should be trimmed or not.

If you want to handle that generality, it would require a more complex implementation, which checks whether the first/last character is one half of a code point that is encoded as 2 16-bit Java characters, and pass a 32-bit int to trim?, or something similar to that.

I have no objections if these API enhancements are made without enabling testing against an arbitrary Unicode code point. In the past, similar suggestions have been rejected in Clojure's built-in lib, e.g. CLJ-945

Comment by Tamas Szabo [ 28/Jan/16 1:56 AM ]

Yes, the UTF-16 encoding and Character representing either a codepoint or a half-codepoint is a bit of a mess, isn't it?

In the Java String and Character API's the methods that accept char, handle only characters in the Basic Multilingual Plane.
trim? accepts a character, so following the same behavior it will work only for removing characters in the Basic Multilingual Plane.

I think even this would be fine, but additionally because the high/low surrogates and the BMP characters are disjoint, you could actually use the same implementation to remove Unicode code points that aren't in the BMP. You can just say that both the high and low code unit of the codepoint are "unwanted".

Ex:
𝄞 is "\uD834\uDD1E"

user=> (trimr (set " \uD834\uDD1E") "example string  𝄞  ")
"example string"
Comment by Andy Fingerhut [ 28/Jan/16 5:11 AM ]

Agreed, but probably better to anti-recommend such an implementation of trimr for removing such things, because it would also remove only one UTF-16 Java character out of 2 high/low surrogates if it matched a member of the set, even if the other surrogate didn't match anything in the set, which would leave behind a malformed UTF-16 string.

Again, probably best to either not include this in the implementation at all, and at most warn about it in the docs, or to handle it in the implementation by checking for high/low surrogates in the loop(s).

Comment by Tamas Szabo [ 28/Jan/16 6:00 AM ]

Yes, you're right. That solution won't work in all cases, so it can't be recommended.

I am slightly inclined towards having trim? accept chars and work only for removing BMP characters. This will arguably be enough for the majority of the use cases.
The other solution can be used for all use cases, but then trim? will have to accept int, or 2 chars, or a string, so trim? would be less intuitive (although closer to the real world ), and writing those trim? functions would be less user friendly.

That being said, I am happy to change the implementation to do that if it is required.

Currently, I'm not even sure if the enhancement will be accepted or rejected or what the process for that is.





[CLJ-1293] Portable "catch-all" mechanism Created: 05/Nov/13  Updated: 27/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: None

Attachments: Text File CLJ-1293-v001.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Design page: http://dev.clojure.org/display/design/Platform+Errors

CLJS ticket/patch: http://dev.clojure.org/jira/browse/CLJS-661

This patch is more permissive than my patch for CLJS: The CLJS patch ensures :default catch blocks occur between non-default catch blocks and finally blocks, if present. This patch just makes (catch :default ...) a synonym for (catch Throwable ...). I wanted to keep the change to the compiler minimum.

Open Question: Catch Throwable (patch v001 does this) or Exception? Alternatively, a more carefully crafted list of "non-fatal" errors. See Scala's NonFatal pattern extractor: http://www.scala-lang.org/api/current/index.html#scala.util.control.NonFatal$



 Comments   
Comment by Brandon Bloom [ 28/Dec/14 11:33 AM ]

Noticed this switched from "Minor" to "Critical", so I figured I should mention that I later realized that we might want :default to catch Exception instead of Throwable, so as to avoid catching Error subclasses. Javadocs say: "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch." If that's what we actually want, I can provide an updated patch.

Comment by Alex Miller [ 28/Dec/14 2:19 PM ]

Seems like an open question, might be best just to list it as such in the description.

I don't really expect to reach consensus on the ticket or patch right now, just trying to update priorities and raise visibility for discussion with Rich once we get to 1.8.





[CLJ-1790] Error extending protocols to Java arrays Created: 29/Jul/15  Updated: 26/Jan/16

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

Type: Defect Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler, protocols

Attachments: Text File 0001-CLJ-1790-emit-a-cast-to-the-interface-during-procol-.patch    
Patch: Code
Approval: Triaged

 Description   

Root cause appears to be related to how protocols are being handled when used with Java arrays:

e.g. for the protocol implementation:

(extend-protocol mp/PImplementation
  (Class/forName "[Ljava.lang.Object;")
    (implementation-key [m] :object-array)
    (meta-info [m]
      {:doc "Clojure.core.matrix implementation for Java Object arrays"})
    (new-vector [m length] (construct-object-vector (long length)))
    (new-matrix [m rows columns]
      (let [columns (long columns)
            m (object-array rows)]
        (dotimes [i rows]
          (aset m i (construct-object-vector columns)))
        m))
    (new-matrix-nd [m shape]
      (construct-nd shape))
    (construct-matrix [m data]
      (construct-object-array data))
    (supports-dimensionality? [m dims]
      (>= dims 1)))

When called as:

(clojure.core.matrix.protocols/construct-matrix (object-array 1) [1])

Gives exception:

VerifyError (class: clojure/core/matrix$eval10586, method: invokeStatic signature: ()Ljava/lang/Object;) Incompatible object argument for function call  java.lang.Class.getDeclaredConstructors0 (:-2)

Also see: CLJ-1381

Patch: 0001-CLJ-1790emit-a-cast-to-the-interface-during-procol.patch



 Comments   
Comment by Nicola Mometto [ 06/Nov/15 3:53 PM ]

Mike Anderson does 1.8.0-beta2 fix this issue?
Alex Miller if core.matrix is still affected this must be fixed before 1.8.0 as it'd mean that direct linking is still broken

Comment by Nicola Mometto [ 06/Nov/15 6:26 PM ]

I could reproduce the bug with 1.8.0-beta2 btw

Comment by Nicola Mometto [ 06/Nov/15 7:27 PM ]

Apparently this is not a 1.8 regression.

At least 1.6 and 1.7 both manifest the same issue:

Clojure 1.6.0
user=> (defprotocol p (f [_]))
p
user=> (fn [] (f (object-array [])))

VerifyError (class: user$eval15920$fn__15921, method: invoke signature: ()Ljava/lang/Object;) Incompatible object argument for function call  user/eval15920 (form-init9183379085801704163.clj:1)

Comment by Michael Blume [ 06/Nov/15 8:24 PM ]

Do we know why core.matrix works with Clojure 1.6/1.7 then?

Comment by Nicola Mometto [ 06/Nov/15 9:09 PM ]

It doesn't.

Clojure 1.7.0
Java HotSpot(TM) 64-Bit Server VM 1.8.0_60-b27

user=> (require 'clojure.core.matrix.protocols)
nil
user=> (clojure.core.matrix.protocols/construct-matrix (object-array 1) [1])

VerifyError (class: user$eval6935, method: invoke signature: ()Ljava/lang/Object;) Incompatible object argument for function call  java.lang.Class.getDeclaredConstructors0 (Class.java:-2)
user=>

I attached a patch that fixes this issue.
It's caused by the jvm verifier understanding that the object on the stack is an array and thus can never be an instance of the protcol interface, but not being able to understant that the code path leading to the direct protocol interface method invocation can never be reached because of a branch guided by an instance check for that interface on the target

Comment by Mike Anderson [ 06/Nov/15 10:10 PM ]

Apologies, it is possible I just hadn't tested this code path thoroughly before.

It only seems to get triggered in certain circumstances, the following behaviour is interesting:

=> (let [o (identity (object-array 1))]
     (clojure.core.matrix.protocols/dimensionality o))
1
=> (let [o (object-array 1)]
     (clojure.core.matrix.protocols/dimensionality o))
VerifyError (class: clojure/core/matrix$eval17775, method: invokeStatic signature: ()Ljava/lang/Object;) Incompatible object argument for function call  java.lang.Class.getDeclaredConstructors0 (:-2)

Perhaps it only happens when the callsite has type information about the protocol parameter?

Comment by Nicola Mometto [ 07/Nov/15 4:53 AM ]

Correct, apparently the jvm verifier doesn't like situations where we have an array on the stack typed as such, and on a later codepath it is used as target for an invokeinterface even if that path is unreachable because of a previous instance check.

here's an explaination of exactly our case in pseudo bytecode:

..
 load obj // Object[]
 dup
 instanceof SomeInterface
 iftruejmp label1
 pop
 jmp end
label1:
 // here is where the verifier chokes.
 // it can figure out that the target is of type Object[] which can never be a SomeInterface
 // but it cannot figure out that this code path can never be reached because of the previous
 // instance check with jump
 // to fix this we need to insert an explicit checkcast to SomeInterface on the target
 invokeinterface SomeInterface/someMethod
end:
 return




[CLJ-1289] aset-* and aget perform poorly on multi-dimensional arrays even with type hints. Created: 01/Nov/13  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Michael O. Church Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: arrays, performance
Environment:

Clojure 1.5.1.

Dependencies: criterium


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

 Description   

Here's a transcript of the behavior. I don't know for sure that reflection is being done, but the performance penalty (about 1300x) suggests it.

user=> (use 'criterium.core)
nil
user=> (def b (make-array Double/TYPE 1000 1000))
#'user/b
user=> (quick-bench (aget ^"[[D" b 304 175))
WARNING: Final GC required 3.5198021166354323 % of runtime
WARNING: Final GC required 29.172288684474303 % of runtime
Evaluation count : 63558 in 6 samples of 10593 calls.
             Execution time mean : 9.457308 µs
    Execution time std-deviation : 126.220954 ns
   Execution time lower quantile : 9.344450 µs ( 2.5%)
   Execution time upper quantile : 9.629202 µs (97.5%)
                   Overhead used : 2.477107 ns

One workaround is to use multiple agets.

user=> (quick-bench (aget ^"[D" (aget ^"[[D" b 304) 175))
WARNING: Final GC required 40.59820310542545 % of runtime
Evaluation count : 62135436 in 6 samples of 10355906 calls.
             Execution time mean : 6.999273 ns
    Execution time std-deviation : 0.112703 ns
   Execution time lower quantile : 6.817782 ns ( 2.5%)
   Execution time upper quantile : 7.113845 ns (97.5%)
                   Overhead used : 2.477107 ns

Cause: The inlined version only applies to arity 2, and otherwise it reflects.



 Comments   
Comment by Gary Fredericks [ 08/Dec/13 9:28 PM ]

A glance at the source makes it obvious that the hypothesis is correct – the inlined version only applies to arity 2, and otherwise it reflects.

I thought this would be as simple as converting the inline function to be variadic (using reduce), but after trying it I realized this is tricky as you have to generate the correct type hints for each step. E.g., given ^"[[D" the inline function needs to type-hint the intermediate result with ^"[D". This isn't difficult if we're just dealing with strings that begin with square brackets, but I don't know for sure that those are the only possibilities.

Comment by Yaron Peleg [ 13/Feb/14 4:44 AM ]

Bump. I just got bitten bad by this.

There are two seperate issues here:
1) (aget 2d-array-doubles 0 0 ) doesn't emit a reflection warning.
2) It seems like the compiler has enough information to avoid the reflective call here.

Note this gets exp. worse as number of dimensions grows, i.e (get doubles3d 0 0 0)
will be 1M slower, etc' Not true, unless you iterate over all elements. it's
simply n_dims*1000x per lookup.

Nasty surprise, especially considering you often go to primitive arrays for speed,
and a common use case is an inner loop(s) that iterate over arrays.

Comment by Gary Fredericks [ 13/Feb/14 7:08 AM ]

I can probably take a stab at this.

Comment by Gary Fredericks [ 13/Feb/14 8:34 PM ]

I think the reflection warning problem is pretty much impossible to solve without changing code elsewhere in the compiler, because the reflection done in aget is a different kind than normal clojure reflection – it's explicitly in the function body rather than emitted by the compiler. Since the compiler isn't emitting it, it doesn't reasonably know it's even there. So even if aget is fixed for other arities, you still won't get the warning when it's not inlined.

I can imagine some sort of metadata that you could put on a function telling the compiler that it will reflect if not inlined. Or maybe a more generic not-inlined warning?

The global scope of adding another compiler flag seems about balanced by the seriousness of array functions not being able to warn on reflection.

Comment by Gary Fredericks [ 13/Feb/14 8:52 PM ]

Attached CLJ-1289-p1.patch which simply inlines variadic calls to aget. It assumes that if it sees a :tag on the array arg that is a string beginning with [, it can assume that the return value from one call to aget can be tagged with the same string with the leading [ stripped off.

I'm not a jvm expert, but having read through the spec a little bit I think this is a reasonable assumption.

Comment by Alex Miller [ 14/Feb/14 3:34 PM ]

I think this probably is actually true, but a more official way to ask that question would be to get the array class and ask for Class.getComponentType() (and less janky than string munging).

Comment by Gary Fredericks [ 14/Feb/14 3:40 PM ]

How would you get the array class based on the :tag type hint?

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

I see (-> s (Class/forName) (.getComponentType) (.getName)) does the same thing – is that route preferred, or is there another one?





[CLJ-1743] Avoid compile-time static initialization of classes when using inheritance Created: 02/Jun/15  Updated: 26/Jan/16

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

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

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

 Description   

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

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

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

Patch: clj-1743-2.patch

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

Demo.java
package clojure_static_initialization;

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

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

unknown.

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

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





[CLJ-401] Add seqable? predicate Created: 13/Jul/10  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: None

Approval: Triaged

 Description   

Many people have found a need for this function and one exists in clojure.core.incubator that is sometimes used and/or copied elsewhere:

https://github.com/clojure/core.incubator/blob/master/src/main/clojure/clojure/core/incubator.clj#L83

This predicate would be valuable to have as it is not a simple check on Seqable since RT.seq() covers a number of additional cases. Alternatively, there could be a protocol for this that could be extended to both Seqable as well as other supported Java use cases turning this into a satisfies? check.

Old prior discussion (although this also comes up regularly on #clojure):



 Comments   
Comment by Assembla Importer [ 24/Aug/10 9:19 AM ]

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

Comment by Jeremy Heiler [ 26/Jul/14 5:37 PM ]

A reference to the implementation in contrib: https://github.com/clojure/clojure-contrib/blob/master/modules/core/src/main/clojure/clojure/contrib/core.clj#L78

It seems like that the only thing that is inconsistent with RT.seqFrom is that seqable? checks for String instead of CharSequence.

Comment by Alex Miller [ 03/Aug/15 10:11 AM ]

In the proposed patch referenced in the ticket above, if seqable? could be used in place of sequential? flatten could be more powerful and work with maps/sets/java collections. Here's how it would look:

(defn flatten [coll] 
  (lazy-seq 
    (when-let [coll (seq coll)] 
      (let [x (first coll)] 
        (if (seqable? x) 
          (concat (flatten x) (flatten (next coll))) 
          (cons x (flatten (next coll))))))))

And an example:

user=> (flatten #{1 2 3 #{4 5 {6 {7 [8 9 10 #{11 12 (java.util.ArrayList. [13 14 15]) (int-array [16 17 18])}]}}}}) 
(1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18)




[CLJ-1611] clojure.java.io/pushback-reader Created: 08/Dec/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: io, reader

Attachments: Text File drupp-clj-1611-2.patch     Text File drupp-clj-1611.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Whereas

  • clojure.core/read and clojure.edn/read require a PushbackReader;
  • clojure.java.io/reader produces a BufferedReader, which isn't compatible;
  • the hazard has tripped folks up for years[1];
  • clojure.java.io is pure sugar anyway (and would not be damaged by the addition of a little bit more);
  • clojure.java.io's very existence suggests suitability and fitness for use (wherein by the absence of a read-compatible pushback-reader it falls short);

i.e., in the total absence of clojure.java.io it would not seem "hard" to use clojure.edn, but in the presence of clojure.java.io and its "reader" function, amidst so much else in the API that does fit together, one keeps thinking one is doing it wrong;

and

  • revising the "read" functions to make their own Pushback was considered but rejected [2];

Therefore let it be suggested to add clojure.java.io/pushback-reader, returning something consumable by clojure.core/read and clojure.edn/read.

[1] The matter was discussed on Google Groups:

(2014, "clojure.edn won't accept clojure.java.io/reader?") https://groups.google.com/forum/#!topic/clojure/3HSoA12v5nc

with a reference to an earlier thread

(2009, "Reading... from a reader") https://groups.google.com/forum/#!topic/clojure/_tuypjr2M_A

[2] CLJ-82 and the 2009 message thread



 Comments   
Comment by David Rupp [ 10/Jan/15 4:05 PM ]

Attached patch drupp-clj-1611.patch implements clojure.java.io/pushback-reader as requested.

Comment by David Rupp [ 10/Jan/15 4:07 PM ]

Note that you can always import java.io.PushbackReader and do something like (PushbackReader. (reader my-thing)) yourself; that's really all the patch does.

Comment by Phill Wolf [ 11/Jan/15 7:54 AM ]

clojure.java.io/reader is idempotent, while the patch of 10-Jan-2015 re-wraps an existing PushbackReader twice: first with a new BufferedReader, then with a new PushbackReader.

Leaving a given PushbackReader alone would be more in keeping with the pattern of clojure.java.io.

It also needs a docstring. If pushback-reader were idempotent, the docstring's opening phrase could echo clojure.java.io/reader's, e.g.: Attempts to coerce its argument to java.io.PushbackReader; failing that, (bla bla bla).

Comment by David Rupp [ 11/Jan/15 11:14 AM ]

Adding drupp-clj-1611-2.patch to address previous comments.





[CLJ-308] protocol-ize with-open Created: 21/Apr/10  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: io

Attachments: Text File 0001-Added-ClosableResource-protocol-for-with-open.patch    
Patch: Code
Approval: Triaged

 Description   

Good use (and documentation example) of protocols: make with-open aware of a Closable protocol for APIs that use a different close convention. See http://groups.google.com/group/clojure/browse_thread/thread/86c87e1fc4b1347c



 Comments   
Comment by Assembla Importer [ 24/Aug/10 4:39 PM ]

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

Comment by Tassilo Horn [ 23/Dec/11 5:11 AM ]

Added a CloseableResource protocol and extended it on java.io.Closeable (implemented by all Readers, Writers, Streams, Channels, Sockets). Use it in with-open.

All tests pass.

Comment by Tassilo Horn [ 23/Dec/11 7:14 AM ]

Seems to be related to Scopes (http://dev.clojure.org/jira/browse/CLJ-2).

Comment by Tassilo Horn [ 08/Mar/12 3:59 AM ]

Updated patch.

Comment by Andy Fingerhut [ 02/Apr/12 12:11 PM ]

Patch 0001-Added-ClosableResource-protocol-for-with-open.patch dated 08/Mar/12 applies, builds, and tests cleanly on latest master as of Apr 2 2012. Tassilo has signed a CA.

Comment by Tassilo Horn [ 13/Apr/12 11:23 AM ]

Updated patch to apply cleanly against master again.

Comment by Brandon Bloom [ 22/Jul/14 9:00 PM ]

I looked up this ticket because I ran in to a reflection warning: with-open does not hint it's binding with java.io.Closeable

Some feedback on the patch:

1) This is a breaking change for anyone relying on the close method to be duck-typed.

2) CloseableResource is a bit long. clojure.core.protocols.Closeable is plenty unambiguous.

3) Rather than extending CloseableResource to java.io.Closeable, you can use the little known (undocumented? unsupported?) :on-interface directive:

(defprotocol Closeable
  :on-interface java.io.Closeable
  (close [this]))

That would perform much better than the existing patch.

Comment by Tassilo Horn [ 23/Jul/14 7:12 AM ]

Hi Brandon, two questions:

Could 1) be circumvented somehow by providing a default implementation somehow? I guess the protocol could be extended upon Object with implementation (.close this), but that would give a reflection warning since Object has no close method. Probably one could extend upon Object and in the implementation search a "close" method using java.lang.reflect and throw an exception if none could be found?

Could you please tell me a bit more about the :on-interface option? How does that differ from extend? And how do I add the implementation, i.e., (.close this) with that option?





[CLJ-1239] faster, more flexible dispatch for clojure.walk Created: 29/Jul/13  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 8
Labels: walk

Attachments: Text File 0001-CLJ-1239-protocol-dispatch-for-clojure.walk.patch     Text File 0002-CLJ-1239-protocol-dispatch-for-clojure.walk.patch    
Patch: Code
Approval: Triaged

 Description   

The conditional dispatch in clojure.walk is slow and not open to extension. Prior to CLJ-1105 it did not support records.

Approach: Reimplement clojure.walk using protocols. The public API does not change. Users can extend the walk protocol to other types (for example, Java collections) if desired. As in CLJ-1105, this version of clojure.walk supports records.

Patch: 0002-CLJ-1239-protocol-dispatch-for-clojure.walk.patch

Performance: My tests indicate this is 1.5x-2x the speed of the original clojure.walk. See https://github.com/stuartsierra/clojure.walk2 for benchmarks.

Risks: This approach carries some risk of breaking user code that relied on type-specific behavior of the old clojure.walk. When running the full Clojure test suite, I discovered (and fixed) some breakages that did not show up in clojure.walk's unit tests. See, for example, commit 730eb75 in clojure.walk2



 Comments   
Comment by Vjeran Marcinko [ 19/Oct/13 12:32 PM ]

It looks, as it is now, that walking the tree and replacing forms doesn't preserve original meta-data contained in data structures.

Comment by Andy Fingerhut [ 23/Nov/13 1:11 AM ]

Patch 0001-CLJ-1239-protocol-dispatch-for-clojure.walk.patch no longer applies cleanly to latest Clojure master since the patch for CLJ-1105 was committed on Nov 22, 2013. From the description, it appears the intent was either that patch or this one, not both, so I am not sure what should happen with this patch, or even this ticket.

Comment by Alex Miller [ 23/Nov/13 1:52 AM ]

This patch and ticket are still candidates for future release.

Comment by Stuart Sierra [ 20/Dec/13 9:14 AM ]

Added new patch that applies on latest master after CLJ-1105.

Comment by Chouser [ 27/Feb/14 10:26 AM ]

The way this patch behaves can be surprising compared to regular maps:

(clojure.walk/prewalk-replace {[:a 1] nil} {:a 1, :b 2})
;=> {:b 2}

(defrecord Foo [a b])
(clojure.walk/prewalk-replace {[:a 1] nil} (map->Foo {:a 1, :b 2}))
;=> #user.Foo{:a 1, :b 2}

Note how the [:a 1] entry is removed from the map, but not from the record.

Here's an implementation that doesn't suffer from that problem, though it does scary class name munging instead: https://github.com/LonoCloud/synthread/blob/a315f861e04fd33ba5398adf6b5e75579d18ce4c/src/lonocloud/synthread/impl.clj#L66

Perhaps we could add to the defrecord abstraction to support well the kind of things that synthread code is doing clumsily, and then walk could take advantage of that.

Comment by Alex Miller [ 27/Feb/14 2:11 PM ]

@Chouser, can you file a new ticket related to this? It's hard to manage work on something from comments on a closed ticket.

Comment by Alex Miller [ 27/Feb/14 3:54 PM ]

@Chouser - Never mind! I was thinking this was the change that went into 1.6. Carry on.

Comment by Nicola Mometto [ 27/Feb/14 5:17 PM ]

Alex, for what it matters clojure-1.6.0 after CLJ-1105 exibits the same behaviour as described by Chouser for this patch





[CLJ-1527] Clarify and align valid symbol and keyword rules for Clojure (and edn) Created: 18/Sep/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: reader

Approval: Triaged

 Description   

Known areas of under-specificity (http://clojure.org/reader#The%20Reader--Reader%20forms):

  • symbols (and keywords) description do not mention constituent characters that are currently in use by Clojure functions such as <, >, =, $ (for Java inner classes), & (&form and &env in macros), % (stated to be valid in edn spec)
  • keywords currently accept leading numeric characters which is at odds with the spec - see CLJ-1286

References:



 Comments   
Comment by Andy Fingerhut [ 17/Oct/14 2:13 AM ]

The Clojure reader documentation also does not mention the following symbols as valid constituent characters. They are all mentioned as valid symbol constituent characters in the EDN readme here: https://github.com/edn-format/edn#symbols

dollar sign - used in Clojure/JVM to separate Java subclass names from class names, e.g. java.util.Map$Entry
percent sign - not sure why this is part of edn spec. In Clojure it seems only to be used inside #() for args like % %1 %&
ampersand - like in &form and &env in macro definitions
equals - clojure.core/= and many others
less-than - clojure.core/< clojure.core/<=
greater-than - clojure.core/> clojure.core/>=

I don't know whether Clojure and edn specs should be the same in this regard, but it seemed worth mentioning for this ticket.

Comment by Andy Fingerhut [ 01/Jun/15 12:22 AM ]

Alex, Rich made this comment on CLJ-17 in 2011: "Runtime validation off the table for perf reasons. cemerick's suggestion that arbitrary symbol support will render them valid is sound, but arbitrary symbol support is a different ticket/idea." I am not aware of any tickets that propose the enhancement of allowing arbitrary symbols to be supported by Clojure, e.g. via a syntax like

#|white space and arbitrary #$@)$~))@ chars here|

Do you think it is reasonable to create an enhancement ticket for supporting arbitrary characters in symbols and keywords?

Comment by Alex Miller [ 01/Jun/15 6:36 AM ]

Sure. I looked into this a bit as a digression off of feature expressions and #| has been reserved for this potential use. However, there are many tricky issues with it and I do not expect this to happen soon - more likely to be something we're pushed to do when necessary for some other reason.

Comment by Herwig Hochleitner [ 01/Jun/15 8:46 AM ]

Wrong ticket, but to anybody thinking about #|arbitrary symbols (or strings)|, please do consider making the delimiters configurable, as in mime multipart.

Comment by Andy Fingerhut [ 01/Jun/15 8:54 AM ]

I've created a design page for now. I'm sure it does not list many of the tricky issues you have found. I'd be happy to take a shot at documenting them if you have any notes you are willing to share.

http://dev.clojure.org/pages/viewpage.action?pageId=11862058

Comment by Andy Fingerhut [ 01/Jun/15 9:01 AM ]

Herwig, can you edit the design page linked in my previous comment, to add a reference or example to precisely how mime multipart allows delimiters to be configurable, and why you believe fixed delimeters would be a bad idea?

Comment by Herwig Hochleitner [ 01/Jun/15 9:46 AM ]

I've commented on the design page.

Comment by Alex Miller [ 13/Jul/15 12:44 PM ]

Removed a couple of issues that have been clarified on the reader page and are no longer issues.

Comment by Nicola Mometto [ 13/Jul/15 12:45 PM ]

Related to CLJ-1530

Comment by Adam Frey [ 15/Jul/15 11:55 AM ]

Related to this: The Clojure reader will not accept symbols and keywords that contain consecutive colons (See LispReader.java), although that is permitted by the current EDN spec. Here is a GitHub issue regarding consecutive colons. I would like to qualify why consecutive colons are disallowed, and sync up the Clojure Reader and the EDN spec on this.

Comment by Herwig Hochleitner [ 31/Jul/15 8:03 AM ]

The updated reader spec says that a symbol can contain a single / to separate the namespace. It also mentions a bare / to be the division function.
So what about clojure.core//? That still got to be a readable symbol right? So is that an exception to the 'single /' rule?
Will foo.bar// also be readable? What about foo//bar?

Comment by Francis Avila [ 10/Sep/15 9:26 AM ]

Another source of ambiguity I see is that it's unclear whether the first colon of a keyword is the first character of the keyword (and therefore of the symbol) or whether it is something special and the spec really describes what happens from the second character onward. This matters because the specification for a keyword is (in both edn and reader specs) given in terms of differences from symbols. I think many of the strange keyword edge cases (including legality of :1 vs :a/1) stem from this ambiguity, and different tickets/patches seem to choose one or the other underlying assumption. See this comment for more examples.

Possibly we can use tagged literals for keywords and symbols to create or print these forms when they are not readable and simplify the reader spec for their literal forms. E.g. instead of producing complicated parse rules to ensure clojure.core// or :1 are legal, just make the literal form simple and have users write something like #sym["clojure.core" "/"] or #kyw "1" (and have the printer print these) when they hit these edge cases.

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

I would say : (and : are syntactic markers and the spec describes the characters following it. But I agree it would be nice for this to be more explicit. The (incorrect) regex in LispReader does not help either.

The tagged literal idea is an interesting alternative to the | | syntax that has been reserved for possible future support for invalid characters in keywords and symbols. But I think the idea is out of scope for this ticket, which is really about clarifying the spec.





[CLJ-1522] Enhance multimethods metadata Created: 08/Sep/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 17
Labels: metadata

Approval: Triaged

 Description   

I think that multimethod metadata can be extended a bit with some property indicating the var in question is referring to a multimethod (we have something similar for macros) and some default arglists property.

I'm raising this issue because as a tool writer (CIDER) I'm having hard time determining if something is a multimethod (I have to resort to code like (instance? clojure.lang.MultiFn obj) which is acceptable, but not ideal I think (compared to macros and special forms)). There's also the problem that I cannot provide the users with eldoc (function signature) as it's not available in the metadata (this issue was raised on the mailing list as well https://groups.google.com/forum/#!topic/clojure/crje_RLTWdk).

I feel that we really have a problem with the missing arglist and we should solve it somehow. I'm not sure I'm suggesting the best solution and I'll certainly take any solution.



 Comments   
Comment by Bozhidar Batsov [ 09/Sep/14 4:24 AM ]

Btw, I failed to mention this as I thought it was obvious, but I think we should use the dispatch function's arglist in the multimethod metadata.





[CLJ-1298] Add more type predicate fns to core Created: 21/Nov/13  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Alex Fowler Assignee: Unassigned
Resolution: Unresolved Votes: 18
Labels: None

Approval: Triaged

 Description   

Add more built-in type predicates:

1) Definitely missing: (atom? x), (ref? x), (deref? x), (named? x), (map-entry? x), (lazy-seq? x), (boolean?).
2) Very good to have: (throwable? x), (exception? x), (pattern? x).

The first group is especially important for writing cleaner code with core Clojure.



 Comments   
Comment by Alex Miller [ 21/Nov/13 8:42 AM ]

In general many of the existing predicates map to interfaces. I'm guessing these would map to checks on the following types:

atom? = Atom (final class)
ref? = IRef (interface)
deref? = IDeref (interface)
named? = Named (interface, despite no I prefix)
map-entry? = IMapEntry (interface)
lazy-seq? = LazySeq (final class)

throwable? = Throwable
exception? = Exception, but this seems less useful as it feels like the right answer when you likely actually want throwable?
pattern? = java.util.regex.Pattern

Comment by Alex Fowler [ 21/Nov/13 9:02 AM ]

Yes, they do, and sometimes the code has many checks like (instance? clojure.lang.Atom x). Ok, you can write a little function (atom? x) but it has either to be written in all relevant namespaces or required/referred there from some extra namespace. All this is just a burden. For example, we have predicates like (var? x) or (future? x) which too map to Java classes, but having them abbreviated often makes possible to write a cleaner code.

I feel the first group to be especially significant for it being about core Clojure concepts like atom and ref. Having to fall to manual Java classes check to work with them feels inorganic. Of course we can, but why then do we have (var? x), (fn? x) and other? Imagine, for example:

(cond
(var? x) (...)
(fn? x) (...)
(instance? clojure.lang.Atom x) (...)
(or (instance? clojure.lang.Named x) (instance? clojure.lang.LazySeq x)) (...))

vs

(cond
(var? x) (...)
(fn? x) (...)
(atom? x) (...)
(or (named? x) (lazy-seq? x)) (...))

The second group is too, essential since these concepts are fundamental for the platform (but you're right with the (exception? x) one).

Comment by Alex Fowler [ 22/Nov/13 6:35 AM ]

Also, obviously I missed the (boolean? x) predicate in the original post. Did not even guess it is absent too until I occasionally got into it today. Currently the most clean way we have is to do (or (true? x) (false? x)). Needles to say, it looks weird next to the present (integer? x) or (float? x).

Comment by Brandon Bloom [ 22/Jul/14 1:02 AM ]

Predicates for core types are also very useful for portability to CLJS.

Comment by Brandon Bloom [ 22/Jul/14 1:05 AM ]

I'd be happy to provide a patch for this, but I'd prefer universal interface support where possible. Therefore, this ticket, in my mind, is behind http://dev.clojure.org/jira/browse/CLJ-803 etc.

Comment by Alex Miller [ 22/Jul/14 6:12 AM ]

I don't think it's worth making a ticket for this until Rich has looked at it and determined which parts are wanted.

Comment by Alex Miller [ 02/Dec/14 4:33 PM ]

Someone asked about a boolean? predicate, so throwing this one on the list...

Comment by Reid McKenzie [ 02/Dec/14 4:51 PM ]

uuid? maybe. UUIDs have a bit of a strange position in that we have special printer handling for them built into core implying that they are intentionally part of Clojure, but there is no ->UUID constructor and no functions in core that operate on them so I could see this one being specifically declined.

Comment by Brandon Bloom [ 03/May/15 2:50 PM ]

This has been troubling me again with my first cljc project. So, I've added a whole bunch of tickets (with patches!) for individual predicates in both CLJ and CLJS.

Comment by Alex Miller [ 03/May/15 5:35 PM ]

As I said above, I don't want to mess with specific patches or tickets on this until Rich gets a look at this and we decide which stuff should and should not be included. So I'm going to ignore your other tickets for now...

Comment by Nicola Mometto [ 24/Nov/15 4:08 PM ]

map-entry? is included since 1.8





[CLJ-304] clojure.repl/source does not work with deftype Created: 20/Apr/10  Updated: 26/Jan/16

Status: In Progress
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 20
Labels: repl

Approval: Triaged

 Description   

clojure.repl/source does not work on a deftype

user> (deftype Foo [a b])
user.Foo
user> (source Foo)
Source not found

Cause: deftype creates a class but not a var so no file/line info is attached anywhere.

Approach:

Patch:

Screened by:



 Comments   
Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

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

Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

chouser@n01se.net said: That's a great question. get-source just needs a file name and line number.

If IMeta were a protocol, it could be extended to Class. That implementation could look for a "well-known" static field, perhaps? __clojure_meta or something? Then deftype would just have to populate that field, and get-source would be all set.

Does that plan have any merit? Is there a better place to store a file name and line number?

Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

stu said: Seems like a reasonable idea, but this is going to get back-burnered for now, unless there is a dire use case we have missed.

Comment by Gary Trakhman [ 19/Feb/14 10:31 AM ]

I could use this for cider's file/line jump-around mechanism as well.

With records, I can work around it by deriving and finding the corresponding constructor var, but it's a bit nasty.

Comment by Bozhidar Batsov [ 03/Mar/14 6:37 AM ]

I'd also love to see this fixed.

Comment by Andy Fingerhut [ 03/Mar/14 8:33 AM ]

Bozhidar, voting on a ticket (clicking the Vote link in the right of the page when viewing the ticket) can help push it upwards on listings of tickets by # of votes.

Comment by Bozhidar Batsov [ 19/Sep/14 1:17 PM ]

Andy, thanks for the pointer. They should have made this button much bigger, I hadn't noticed it all until now.

Comment by Alex Miller [ 26/Jan/16 4:12 PM ]

If someone did some work on this (like a patch), I would push it harder.





[CLJ-1629] Improve error message when defn form omits parameter declaration Created: 29/Dec/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Sanel Zukan Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs
Environment:

Reproducible on all platforms and all clojure versions.


Approval: Triaged

 Description   

When defn form is malformed, Clojure compiler will report meaningless error and in combination with function body, can cause really bad experience. Here is the sample:

(defn foo
  "This is docstring."
  (let [i 1]
    (+ i 1)))

It will report:

IllegalArgumentException Parameter declaration "let" should be a vector  clojure.core/assert-valid-fdecl (core.clj:7123)

However, if is written:

(defn foo "bla")

error report makes more sense:

IllegalArgumentException Parameter declaration missing  clojure.core/assert-valid-fdecl (core.clj:7107)


 Comments   
Comment by Michael Blume [ 29/Dec/14 1:39 PM ]

I don't think this is really meaningless – if you replace the symbol let with a vector, say, [i], you get a perfectly valid function definition

(defn foo
  "This is docstring."
  ([i] [i 1]
    (+ i 1)))
Comment by Sanel Zukan [ 29/Dec/14 2:41 PM ]

Yes and maybe make sense for this case. But in general, the report is misleading for common defn forms (how often you will see function definitions written this way, unless you want multi-arity function) and should have the same report as for second sample; in both cases it is the same cause.





[CLJ-1107] 'get' should throw exception on non-Associative argument Created: 13/Nov/12  Updated: 26/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 17
Labels: checkargs

Attachments: Text File 0001-CLJ-1107-Throw-exception-for-get-called-on-unsupport.patch     Text File 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch     Text File clj-1107-throw-on-unsupported-get-v4.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The implementation of clojure.core/get returns nil if its argument is not an associative collection.

This behavior can obscure common programmer errors such as:

(def a (atom {:a 1 :b 2})

(:foo a)   ; forgot to deref a
;;=> nil

Calling get on something which is neither nil nor an Associative collection is almost certainly a bug, and should be indicated by an exception.

CLJ-932 was accepted as a similar enhancement to clojure.core/contains?

Patch: 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch

Approach: Throw IllegalArgumentException as final fall-through case in RT.getFrom instead of returning nil.

Also see: CLJ-969



 Comments   
Comment by Andy Fingerhut [ 24/May/13 12:31 PM ]

Patch clj-1107-throw-on-get-for-unsupported-types-patch-v2.txt dated May 24 2013 is identical to 0001-CLJ-1107-Throw-exception-for-get-called-on-unsupport.patch dated Nov 13 2012, except it applies cleanly to latest master. A recent commit for CLJ-1099 changed many IllegalArgumentException occurrences to Throwable in the tests, which is the only thing changed in this updated patch.

Comment by Andy Fingerhut [ 30/Jan/14 5:01 PM ]

Patch clj-1107-throw-on-get-for-unsupported-types-patch-v2.txt applied cleanly to latest Clojure master as of Jan 23 2014, but no longer does with commits made to Clojure between then and Jan 30 2014. I have not checked to see how difficult or easy it may be to update this patch.

Comment by Stuart Sierra [ 11/Feb/14 7:23 AM ]

New patch 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch created from master at 5cc167a.

Comment by Andy Fingerhut [ 26/Mar/14 11:55 AM ]

Patch clj-1107-throw-on-unsupported-get-v4.patch dated Mar 26 2014 is identical to Stuart Sierra's patch 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch, and retains his authorship. The only difference is in one line of diff context required in order to make it apply cleanly to latest master.