<< Back to previous view

[CLJ-1232] Functions with non-qualified return type hints force import of hinted classes when called from other namespace Created: 18/Jul/13  Updated: 17/Apr/14

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

Type: Defect Priority: Major
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: typehints


 Description   

You can add a type hint to function arglists to indicate the return type of a function like so.

user> (import '(java.util List))
java.util.List
user> (defn linkedlist ^List [] (java.util.LinkedList.))
#'user/linkedlist
user> (.size (linkedlist))
0

The problem is that now when I call `linkedlist` exactly as above from another namespace, I'll get an exception because java.util.List is not imported in there.

user> (in-ns 'user2)
#<Namespace user2>
user2> (refer 'user)
nil
user2> (.size (linkedlist))
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: List, compiling:(NO_SOURCE_PATH:1:1)
user2> (import '(java.util List)) ;; Too bad, need to import List here, too.
java.util.List
user2> (.size (linkedlist))
0

There are two workarounds: You can import the hinted type also in the calling namespace, or you always use fully qualified class names for return type hints. Clearly, the latter is preferable.

But clearly, that's a bug that should be fixed. It's not in analogy to type hints on function parameters which may be simple-named without having any consequences for callers from other namespaces.



 Comments   
Comment by Andy Fingerhut [ 16/Apr/14 3:47 PM ]

To make sure I understand, Nicola, in this ticket you are asking that the Clojure compiler change behavior so that the sample code works correctly with no exceptions, the same way as it would work correctly without exceptions if one of the workarounds were used?

Comment by Tassilo Horn [ 17/Apr/14 12:18 AM ]

Hi Andy. Tassilo here, not Nicola. But yes, the example should work as-is. When I'm allowed to use type hints with simple imported class names for arguments, then doing so for return values should work, too.





[CLJ-1405] clojure runtime does not work with onejar-maven-plugin Created: 16/Apr/14  Updated: 16/Apr/14

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

Type: Defect Priority: Minor
Reporter: Sean Shubin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

I was able to repeat this problem on windows, linux, and osx


Attachments: Text File onejar-maven-plugin-fix.patch    
Patch: Code

 Description   

I have created a sample project, steps to repeat the problem, and a proposed patch here:
https://github.com/SeanShubin/clojure-one-jar

I have also attached my proposed patch to this ticket, and pasted the relevant portion of README.md below:

clojure-one-jar
===============

Sample project to demonstrate problem with combining clojure runtime with onejar-maven-plugin

Steps to repeat the problem
===========================

  • mvn package
  • java -jar target/greeter.jar

Behavior before patch is applied
================================
Exception in thread "main" java.lang.reflect.InvocationTargetException
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:483)
at com.simontuffs.onejar.Boot.run(Boot.java:340)
at com.simontuffs.onejar.Boot.main(Boot.java:166)
Caused by: java.lang.ExceptionInInitializerError
at com.seanshubin.clojure_one_jar.Greeter.main(Greeter.java:12)
... 6 more
Caused by: java.lang.NullPointerException
at clojure.lang.RT.lastModified(RT.java:387)
at clojure.lang.RT.load(RT.java:421)
at clojure.lang.RT.load(RT.java:411)
at clojure.lang.RT.doInit(RT.java:447)
at clojure.lang.RT.<clinit>(RT.java:329)
... 7 more

Behavior after patch is applied
===============================
No need to call RT.init() anymore
Hello, world!

Patch
=====
I applied this patch to my local copy of clojure 1.7.0-master-SNAPSHOT
https://github.com/SeanShubin/clojure-one-jar/blob/master/onejar-maven-plugin-fix.patch



 Comments   
Comment by Alex Miller [ 16/Apr/14 9:45 PM ]

I think this is a dupe of CLJ-971 - can you verify?

Comment by Sean Shubin [ 16/Apr/14 9:49 PM ]

Yes, I looked over CLJ-971 and can confirm this matches my observations regarding why this is happening.

Comment by Sean Shubin [ 16/Apr/14 9:54 PM ]

My solution is a bit different in that it still tries to get the date of the file rather than defaulting to the date of the entire jar. I don't have a solid enough understanding of the calling code to know if this is important.

Comment by Alex Miller [ 16/Apr/14 10:17 PM ]

FYI, to consider your patch, I would need you to sign a Contributor Agreement - see http://clojure.org/contributing for details. I do think that is an area that something is needed, will need to assess the options a bit more.

Comment by Sean Shubin [ 16/Apr/14 10:56 PM ]

Sure thing, I just signed and sent a Contributor Agreement, per the instructions at http://clojure.org/contributing





[CLJ-971] Jar within a jar throws a runtime error Created: 10/Apr/12  Updated: 16/Apr/14

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

Type: Defect Priority: Major
Reporter: Ron Romero Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Maven using the one-jar plugin


Attachments: Text File clj-971-1.patch     Text File clj-971-2.patch    
Patch: Code
Approval: Triaged

 Description   

I've created two jar files in my multi-project Maven setup. The first jar is the "engine", and it includes the clojure jar in it. The other jar is the "application". It includes the engine and then packages itself into a one-jar jar file. This means we have a jar within a jar: The "onejar" contains the engine jar, which in turn contains that clojure jar.

I then get an error in the runtime:

Exception in thread "main" java.lang.reflect.InvocationTargetException
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:616)
at com.simontuffs.onejar.Boot.run(Boot.java:340)
at com.simontuffs.onejar.Boot.main(Boot.java:166)
Caused by: java.lang.ExceptionInInitializerError
at com.ziroby.clojure.App.main(App.java:14)
... 6 more
Caused by: java.lang.NullPointerException
at clojure.lang.RT.lastModified(RT.java:374)
at clojure.lang.RT.load(RT.java:408)
at clojure.lang.RT.load(RT.java:398)
at clojure.lang.RT.doInit(RT.java:434)
at clojure.lang.RT.<clinit>(RT.java:316)
... 7 more

See also my Stack Overflow question on this at http://stackoverflow.com/questions/7763480/making-an-executable-jar-that-evals-clojure-strings

In researching it, I've found the problem lies in RT.lastModified, where it tries to determine last modified time by looking at the modified time on the jar file for Clojure. But there's not actually a jar file, since it's embedded in another.

I've found that adding a null check solves the problem. My lastModified looks like this now:

static public long lastModified(URL url, String libfile) throws Exception{
if(url.getProtocol().equals("jar")) { ZipEntry entry = ((JarURLConnection) url.openConnection()).getJarFile().getEntry(libfile); if (entry != null) return entry.getTime(); }

return url.openConnection().getLastModified();
}

This runs successfully.

If you'd prefer, I can submit a patch, or commit directly.



 Comments   
Comment by Anders Sveen [ 11/Nov/13 3:29 AM ]

Would be awesome if you could get this working. Wanting to use som Clojure libs in Java so Leiningen uberjar is not an option right now.

Comment by Paavo Parkkinen [ 24/Nov/13 7:11 AM ]

Took the code change in the description and rolled it into a patch to hopefully push this forward a little bit.

Comment by Alex Miller [ 24/Nov/13 10:06 AM ]

Thanks Paavo - on a quick scan, it looks like in the case you're interested in the updated code would now call url.openConnection() twice - perhaps that could be factored out?

Comment by Paavo Parkkinen [ 25/Nov/13 6:19 AM ]

New patch with duplicate calls to url.openConnection() factored out.

Comment by Gleb Kanterov [ 13/Dec/13 4:30 AM ]

I had the same issue, adding following line to manifest file worked for me

One-Jar-URL-Factory: com.simontuffs.onejar.JarClassLoader$OneJarURLFactory
Comment by Stuart Halloway [ 31/Jan/14 1:04 PM ]

Can we get a test showing the normal path and the can't-read-inside path?





[CLJ-1325] Add *warn-on-boxed-math* warning Created: 16/Jan/14  Updated: 15/Apr/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 1
Labels: errormsgs, math

Attachments: File boxed.diff     Text File boxedmath.txt    
Patch: Code
Approval: Vetted

 Description   

Currently, it is difficult to tell that the compiler is using boxed math unless you look at the generated bytecode. Adding warn-on-boxed-math warning (similar in use to warn-on-reflection) to warn when these calls are being made.

Approach: In the compiler, when compiling a StaticMethodExpr that is specifying a call into certain boxed math methods on Number, and if the warning is enabled, print the warning. The list of calls would need to be in a boxed math "black list" which would probably be created manually.

Patch: The attached patch is an incomplete sketch of the solution. It is not yet specific enough in determining whether the particular call to Numbers is doing boxed math - it merely looks for an Object argument, which does catch a large set of the actual calls, but may also ensnare some calls that should not be included. One option would be to mark the offending methods in Numbers with an annotation that could be checked in isBoxedMath.

Screened by:



 Comments   
Comment by Alex Miller [ 14/Apr/14 10:56 PM ]

Moving to 1.7.

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

List of methods in Numbers and whether they should be considered "boxed math" or not, with some questions.





[CLJ-1404] clojure.core/vals returns nil on an empty map instead of an empty sequence Created: 14/Apr/14  Updated: 14/Apr/14  Resolved: 14/Apr/14

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

Type: Defect Priority: Trivial
Reporter: Satshabad Khalsa Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Is this a bug? maybe I just don't understand. The documentation says: Returns a sequence of the map's values. Is nil a sequence?

This caused an unexpected nil to propagate through a bunch of list processing stuff.



 Comments   
Comment by Alex Miller [ 14/Apr/14 10:54 PM ]

An empty sequence is represented by nil, so this is consistent. For example: (seq (range 0)) => nil





[CLJ-1403] ns-resolve might throw ClassNotFoundException but should return nil Created: 14/Apr/14  Updated: 14/Apr/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

The doc of ns-resolve states that in case the symbol cannot be resolved, it should return nil.

user=> (doc ns-resolve)
-------------------------
clojure.core/ns-resolve
([ns sym] [ns env sym])
  Returns the var or Class to which a symbol will be resolved in the
  namespace (unless found in the environment), else nil.  Note that
  if the symbol is fully qualified, the var/Class to which it resolves
  need not be present in the namespace.
nil

However if the symbol contains dots and is not a resolvable Class, a ClassNotFoundException is thrown

user=> (ns-resolve *ns* 'foo.bar)
ClassNotFoundException foo.bar  java.net.URLClassLoader$1.run (URLClassLoader.java:372)
user=> (pst *e)
ClassNotFoundException foo.bar
	java.net.URLClassLoader$1.run (URLClassLoader.java:372)
	java.net.URLClassLoader$1.run (URLClassLoader.java:361)
	java.security.AccessController.doPrivileged (AccessController.java:-2)
	java.net.URLClassLoader.findClass (URLClassLoader.java:360)
	clojure.lang.DynamicClassLoader.findClass (DynamicClassLoader.java:61)
	java.lang.ClassLoader.loadClass (ClassLoader.java:424)
	java.lang.ClassLoader.loadClass (ClassLoader.java:357)
	java.lang.Class.forName0 (Class.java:-2)
	java.lang.Class.forName (Class.java:340)
	clojure.lang.RT.classForName (RT.java:2065)
	clojure.lang.Compiler.maybeResolveIn (Compiler.java:6963)
	clojure.core/ns-resolve (core.clj:4026)
nil


 Comments   
Comment by Alex Miller [ 14/Apr/14 2:07 PM ]

Can you include the (pst *e) ?

Comment by Nicola Mometto [ 14/Apr/14 2:10 PM ]

Added result of (pst *e) in the description





[CLJ-1134] star-directive in clojure.pprint/cl-format with an at-prefix ("~n@*") do not obey its specifications Created: 18/Dec/12  Updated: 14/Apr/14

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

Type: Defect Priority: Minor
Reporter: Jean Niklas L'orange Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print

Attachments: Text File clj-1134-star-directive-in-cl-format.txt    
Patch: Code and Test
Approval: Triaged

 Description   

The star-directive in clojure.pprint/cl-format with an at-prefix (~n@*) does not obey its specifications according to Common Lisp the Language, 2nd Edition. There are two bugs within ~n@* as of right now:

  1. When ~n@* is supposed to jump forward over more than one argument, it jumps one step backward as if it had seen ~:*. For instance, (cl-format nil "~D ~3@*~D" 0 1 2 3) will return "0 0" and not "0 3" as expected.
  2. When ~@* is seen, the formatter is supposed to jump to the first argument (as n defaults to 0, see specification linked above). However, whenever a ~@*-directive is seen, the formatter jumps to the second argument instead.

What (small set of) steps will reproduce the problem?

Inside a clean Clojure repl, perform these steps:

user=> (require '[clojure.pprint :refer [cl-format]])
nil
user=> (cl-format nil "~D ~3@*~D" 0 1 2 3)
"0 0"                                           ;; Expected: "0 3"
user=> (cl-format nil "~D~D~D~D ~@*~D" 0 1 2 3)
"0123 1"                                        ;; Expected: "0123 0"

What is the expected output? What do you see instead?

The expected output is "0 3" and "0123 0", but is "0 0" and "0123 1" as shown above.

What version are you using?

Tested on both 1.4.0 and 1.5.0-beta2, both have the defect described.

Please provide any additional information below.

The format strings which reproduces the problem has been compared with the format function from the Common Lisp implementations SBCL, CLisp and Clozure. All of them print the expected output.



 Comments   
Comment by Jean Niklas L'orange [ 18/Dec/12 9:28 PM ]

Patch attached.

It may be easier to read the changes the patch does from within JIRA instead from the commit message, so I've added it here:

This solves two issues as specified by #CLJ-1134. Issue #1 is solved by doing a
relative jump forward within absolute-reposition in cl_format.clj, line 114 by
switching (- (:pos navigator) position) with (- position (:pos navigator)).

Issue #2 is handled by changing the default n-parameter to * depending on
whether the @-prefix is placed or not. If it is placed, then n defaults to
0, otherwise it defaults to 1.

In addition, new tests have been appended to test_cl_format.clj to ensure the
correctness of this patch. The tests have been tested on the Common Lisp
implementation GNU CLISP 2.49, which presumably handle the ~n@*
correctly. This patch and GNU CLISP returns the same output for each format
call, sans case for printed symbols; Common Lisp has case-insensitive symbols,
whereas Clojure has not.

Comment by Tom Faulhaber [ 14/Apr/14 11:12 AM ]

I walked through this patch and it looks just right. Thanks!

Let's get it applied for 1.7.





[CLJ-1039] Using 'def with metadata {:type :anything} throws ClassCastException during printing Created: 09/Aug/12  Updated: 14/Apr/14

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

Type: Defect Priority: Minor
Reporter: Gunnar Völkel Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Ubuntu, lein 1.7.1 - lein repl


Attachments: Text File CLJ-1039-tolerate-misleading-type-metadata-on-var-wh.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Specific to setting :type meta on a var:

user=> (def ^{:type :anything} mydef 1)
#<main$repl clojure.main$repl@6193b845>
CompilerException java.lang.ClassNotFoundException: main.clj:257, compiling:(NO_SOURCE_PATH:13:20)
ClassCastException clojure.lang.Var cannot be cast to clojure.lang.IObj  clojure.core/with-meta (core.clj:214)
user=> (pst *e)
ClassCastException clojure.lang.Var cannot be cast to clojure.lang.IObj
	clojure.core/with-meta (core.clj:214)
	clojure.core/vary-meta (core.clj:640)
	clojure.core/fn--5420 (core_print.clj:76)
	clojure.lang.MultiFn.invoke (MultiFn.java:232)
	clojure.core/pr-on (core.clj:3392)
	clojure.core/pr (core.clj:3404)
	clojure.core/apply (core.clj:624)
	clojure.core/prn (core.clj:3437)
	clojure.main/repl/read-eval-print--6627 (main.clj:241)
	clojure.main/repl/fn--6636 (main.clj:258)
	clojure.main/repl (main.clj:258)
	clojure.main/repl-opt (main.clj:324)

If it is intended to forbid setting the :type metadata, then there should be an appropriate error message instead of the ClassCastException.

Cause: This is caused by the printer dispatch function

(defmulti print-method (fn [x writer]
                         (let [t (get (meta x) :type)]
                           (if (keyword? t) t (class x)))))

which ends up calling the default dispatch, which tries to vary-meta.

Solution:

Patch:

Screened by:



 Comments   
Comment by Stuart Halloway [ 10/Aug/12 1:40 PM ]

This is caused by the printer dispatch function

(defmulti print-method (fn [x writer]
                         (let [t (get (meta x) :type)]
                           (if (keyword? t) t (class x)))))

which ends up calling the default dispatch, which tries to vary-meta.

Comment by Steve Miner [ 14/Apr/14 10:13 AM ]

The :type metadata is used internally by Clojure. For the situation in this bug report, you have to take responsibility for providing a print-method if you put :type metatdata on your var.

This is not a good example, but it shows one way to work around the bug:

(defmethod print-method :anything [obj w] (print-method {:anything @obj} w))
Comment by Steve Miner [ 14/Apr/14 10:21 AM ]

On the other hand, the :default print-method probably should be more robust. I think a check for (instance? clojure.lang.IObj o) before calling vary-meta would be appropriate. Added a patch that calls print-simple if the o isn't an IObj. That works around the issue for Var, and seems reasonable for other exotic types. The only downside I can imagine is if someone had a custom print-method but accidentally had a typo in their :type metadata, they will no longer get an error. This was an edge case to begin with so that probably doesn't matter.





[CLJ-1185] `reductions should respect `reduced Created: 16/Mar/13  Updated: 14/Apr/14

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

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File CLJ-1181-v001.patch    
Patch: Code
Approval: Triaged

 Description   

This returns 16:

(reduce (fn [acc x]
          (let [x' (* x x)]
            (if (> x' 10)
              (reduced x')
              x')))
        (range))

But replacing `reduce with `reductions will never terminate:

(reductions (fn [acc x]
              (let [x' (* x x)]
                (if (> x' 10)
                  (reduced x')
                  x')))
            (range))

This is because `reductions ignores 'clojure.lang.Reduced, it never tests for `reduced?

I know that I only just discovered the `reduced, function, but `reductions is a big part of my debugging process, so it's unfortunate that they don't work together.



 Comments   
Comment by Brandon Bloom [ 16/Mar/13 6:10 PM ]

Attaching patch

Comment by Satshabad Khalsa [ 13/Apr/14 1:53 AM ]

Would love some progress on this!

Comment by Andy Fingerhut [ 13/Apr/14 11:37 AM ]

It isn't guaranteed to help, but it can't hurt to vote on the ticket, and encourage anyone else you know who wants this fixed to vote on it.





[CLJ-1401] CompilerException / IllegalStateException when reloading namespaces Created: 10/Apr/14  Updated: 12/Apr/14

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

Type: Defect Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, errormsgs


 Description   
user> (ns op)
nil
op> (defn * [a b] (clojure.core/* a b))
WARNING: * already refers to: #'clojure.core/* in namespace: op, being replaced by: #'op/*
#'op/*
op> (ns use-op (:require [op :refer :all]))
WARNING: * already refers to: #'clojure.core/* in namespace: use-op, being replaced by: #'op/*
nil
use-op> (ns use-op (:require [op :refer :all]))
IllegalStateException * already refers to: #'op/* in namespace: use-op  clojure.lang.Namespace.warnOrFailOnReplace (Namespace.java:88)
use-op> (clojure.repl/pst *e)
IllegalStateException * already refers to: #'op/* in namespace: use-op
	clojure.lang.Namespace.warnOrFailOnReplace (Namespace.java:88)
	clojure.lang.Namespace.reference (Namespace.java:110)
	clojure.lang.Namespace.refer (Namespace.java:168)
	clojure.core/refer (core.clj:3920)
	use-op/eval2402/loading--4958--auto----2403 (NO_SOURCE_FILE:1)
	use-op/eval2402 (NO_SOURCE_FILE:1)
	clojure.lang.Compiler.eval (Compiler.java:6703)
	clojure.lang.Compiler.eval (Compiler.java:6692)
	clojure.lang.Compiler.eval (Compiler.java:6666)
	clojure.core/eval (core.clj:2927)
	clojure.main/repl/read-eval-print--6625/fn--6628 (main.clj:239)
	clojure.main/repl/read-eval-print--6625 (main.clj:239)

I would expect (at worst) a similar warning to the initial namespace loading, rather than an exception here.



 Comments   
Comment by Alex Miller [ 11/Apr/14 8:26 AM ]

Could you put together a better reproducible test case for this that does not depend on core.matrix? Also, please include the (pst *e) when it occurs.

Comment by Andy Fingerhut [ 11/Apr/14 10:19 AM ]

I have tried the smallest possible Leiningen project I could think of that would cause the warnings about redefinitions, to see if I could get the exception to occur. 'lein new try1' to create the skeleton project, then edit src/try1/core.clj to contain only the following function definitions:

(defn merge
  "This definition of merge replaces clojure.core/merge"
  [x y]
  (- x y))

(defn *
  [x y]
  (* x y))

Then start a REPL with 'lein repl', and I see this behavior:

user=> (require '[try1.core :as c])
WARNING: merge already refers to: #'clojure.core/merge in namespace: try1.core, being replaced by: #'try1.core/merge
WARNING: * already refers to: #'clojure.core/* in namespace: try1.core, being replaced by: #'try1.core/*
nil
user=> (require '[try1.core :as c] )
nil
user=> (require '[try1.core :as c] :reload)
WARNING: merge already refers to: #'clojure.core/merge in namespace: try1.core, being replaced by: #'try1.core/merge
WARNING: * already refers to: #'clojure.core/* in namespace: try1.core, being replaced by: #'try1.core/*
nil

Ths all looks like behavior as I would expect, and I did not see the exception that Mike reports.

It seems that either Ctrl+Alt+L in Counterclockwise does something different than (require ... :reload), or there is something different about Mike's namespace in addition to redefining names in clojure.core that is causing the problem.

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

Marking this as NR for now - would be happy to see it reopened with an easily reproducible test case.

Comment by Mike Anderson [ 12/Apr/14 12:41 AM ]

To reproduce:

(ns op)
(defn * [a b] (clojure.core/* a b)) ;; gives warning
(ns use-op (:require [op :refer :all])) ;; gives warning
(ns use-op (:require [op :refer :all])) ;; gives error!

I believe Counterclockwise is simply loading the namespace again with CTRL-Alt+L, which is causing the ns form to be re-executed.

The docstring implies that ns can be used multiple times ("Sets ns to the namespace named by name (unevaluated), creating it if needed") so I would certainly expect multiple invocations of ns to be a no-op





[CLJ-809] fn's created using defn should not lexical shadow the var that holds them Created: 11/Jun/11  Updated: 11/Apr/14

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

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


 Description   

currently (defn foo [x] (foo x)) expands to something like (def foo
(fn foo [x] (foo x))) so the fn is bound to foo lexically in the scope
of the fn body.

because of this lexical shadowing self calls to fns defined with defn
do not incur the overhead of var dereferencing, I assume this is the
reason it was added.

the lexical shadowing also breaks memoization if you create the
memoized functions via (alter-var-root #'foo memoize) and several
macros here and there emit code in this style, since reusing defn is
the easiest way to get all the good juicy metadata bits like arglists,
etc.

1.3 has changes to eliminate some of the cost of going through the var.

since going through the var is much cheaper now it would be nice to
eliminate the lexical shadowing.

http://groups.google.com/group/clojure-dev/browse_thread/thread/33b52b24616967f?hl=en



 Comments   
Comment by Kevin Downey [ 11/Jun/11 2:40 PM ]

tests indicate that accessing the fn through the var vs. accessing through the lexical binding have very similar access times

https://gist.github.com/1013008

results in

$ java -jar clojure.jar ~/src/lexicaltest.clj
lexical binding run 0
"Elapsed time: 86.739 msecs"
lexical binding run 1
"Elapsed time: 8.062 msecs"
lexical binding run 2
"Elapsed time: 16.182 msecs"
var binding run 0
"Elapsed time: 61.136 msecs"
var binding run 1
"Elapsed time: 40.317 msecs"
var binding run 2
"Elapsed time: 11.641 msecs"
$

Comment by Jason Wolfe [ 13/Jun/11 5:05 PM ]

Here's another test that makes sure no JIT funny business (e.g. dead code elimination) is going on, and also looks at primitive hinted versions.

https://gist.github.com/1023816

var-obj result, time: 14930352 ,"Elapsed time: 965.577 msecs"
lex-obj result, time: 14930352 ,"Elapsed time: 957.741 msecs"
var-prim result, time: 14930352 ,"Elapsed time: 770.411 msecs"
lex-prim result, time: 14930352 ,"Elapsed time: 113.412 msecs"
nil

I'm not sure how to properly hint the var in the var-prim version to get truly comparable results. In any case, this use case should definitely be kept in mind.

Comment by Kevin Downey [ 13/Jun/11 5:35 PM ]

I think the point is we want to make sure the jvm can optimize both just as well, so making the code purposefully unoptimizable is kind of silly, yes?

Comment by Jason Wolfe [ 21/Jun/11 4:41 PM ]

@Kevin In your gist, the return value of foo is not used, and foo has no side effects. A sufficiently smart compiler could optimize the calls out entirely, which would not tell us much about the runtime of foo.

Comment by Aaron Bedra [ 28/Jun/11 6:49 PM ]

Rich: Given the performance testing (in the email thread) what is the next step here?

  • more thorough testing
  • patch?

Are there things that this might break that we need to consider?

Comment by Rich Hickey [ 29/Jul/11 7:39 AM ]

Someone needs to make a patch, and then test perf with and without the patch. These simulated tests aren't necessarily indicative. Naive fib is an ok test once you have a patch. And yes, more things might break - needs careful assessment.

Comment by Nicola Mometto [ 11/Apr/14 5:29 PM ]

If I understand this ticket correctly, it looks like this has been done at some point in time.

Clojure 1.7.0-master-SNAPSHOT
user=> (macroexpand-1 '(defn x []))
(def x (clojure.core/fn ([])))




[CLJ-1192] vec function is substantially slower than into function Created: 06/Apr/13  Updated: 11/Apr/14

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

Type: Defect Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance

Approval: Triaged

 Description   

(vec coll) and (into [] coll) do exactly the same thing. However, due to into using transients, it is substantially faster. On my machine:

(time (dotimes [_ 100] (vec (range 100000))))
"Elapsed time: 732.56 msecs"

(time (dotimes [_ 100] (into [] (range 100000))))
"Elapsed time: 491.411 msecs"

This is consistently repeatable.

Since vec's sole purpose is to transform collections into vectors, it should do so at the maximum speed available.



 Comments   
Comment by Andy Fingerhut [ 07/Apr/13 5:50 PM ]

I am pretty sure that Clojure 1.5.1 also uses transient vectors for (vec (range n)) (probably also some earlier versions of Clojure, too).

Look at vec in core.clj. It checks whether its arg is a java.util.Collection, which lazy seqs are, so calls (clojure.lang.LazilyPersistentVector/create coll).

LazilyPersistentVector's create method checks whether its argument is an ISeq, which lazy seqs are, so it calls PersistentVector.create(RT.seq(coll)).

All 3 of PersistentVector's create() methods use transient vectors to build up the result.

I suspect the difference in run times are not because of transients or not, but because of the way into uses reduce, and perhaps may also have something to do with the perhaps-unnecessary call to RT.seq in LazilyPersistentVector's create method (in this case, at least – it is likely needed for other types of arguments).

Comment by Alan Malloy [ 14/Jun/13 2:17 PM ]

I'm pretty sure the difference is that into uses reduce: since reducers were added in 1.5, chunked sequences know how to reduce themselves without creating unnecessary cons cells. PersistentVector/create doesn't use reduce, so it has to allocate a cons cell for each item in the sequence.

Comment by Gary Fredericks [ 08/Sep/13 1:55 PM ]

Is there any downside to (defn vec [coll] (into [] coll)) (or the inlined equivalent)?

Comment by Ghadi Shayban [ 11/Apr/14 5:13 PM ]

While I agree that there are improvements and possibly low-hanging fruit, FWIW https://github.com/clojure/tools.analyzer/commit/cf7dda81a22f4c9c1fe64c699ca17e7deed61db4#commitcomment-5989545

showed a 5% slowdown from a few callsites in tools.analyzer.

This ticket's benchmark is incomplete in that it covers a single type of argument (chunked range), and flawed as it timing the expense of realizing the range. (That could be a legit benchmark case, but it shouldn't be the only one).

Sorry to rain on a parade. I promise like speed too!





[CLJ-99] GC Issue 95: max-key and min-key evaluate k multiple times for arguments Created: 17/Jun/09  Updated: 11/Apr/14

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

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

Attachments: File clj-99-v1.diff    
Patch: Code and Test

 Description   
Reported by H.Duerer, Mar 13, 2009

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.

Something like the code below would avoid these double calculations (at the
price of generating more ephemeral garbage)

(defn max-key
  "Returns the x for which (k x), a number, is greatest."
  ([k x] x)
  ([k x y] (if (> (k x) (k y)) x y))
  ([k x y & more]
     (second (reduce (fn [x y] (if (> (first x) (first y)) x y))
                     (map #(vector (k %) %) (cons x (cons y more)))))))


 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





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

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


 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





[CLJ-1384] clojure.core/set should use transients Created: 15/Mar/14  Updated: 11/Apr/14

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

Type: Enhancement Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: performance

Attachments: Text File CLJ-1384-p1.patch     File set-bench.tar    
Patch: Code
Approval: Triaged

 Description   

clojure.core/vec calls (more or less) PersistentVector.create(...), which uses a transient vector to build up the result.

clojure.core/set on the other hand, calls PersistentHashSet.create(...), which repeatedly calls .cons on a PersistentHashSet, with all the associated speed/GC issues.

Operation count now w/transients
set 5 1.771924 µs 1.295637 µs
into 5 1.407925 µs 1.402995 µs
set 1000000 2.499264 s 1.196653 s
into 1000000 0.977555 s 1.006951 s


 Comments   
Comment by Gary Fredericks [ 15/Mar/14 10:13 PM ]

PersistentHashSet has six methods for creating sets – one for each combination of {with check, without check} and {array (varargs), List, ISeq}. Each of them does not use transients but could.

I believe clojure.core/set only depends on the (without check, ISeq) version.

Should all of these be changed? Three of them? One of them?

Comment by Andy Fingerhut [ 15/Mar/14 10:21 PM ]

I believe that the 'with check' versions are only intended to be used when reading set literals in Clojure source code, and give an error if there are duplicate elements. If you find examples where those set creation functions are called in other situations, I would be interested to learn about them to find out where my misunderstanding lies, or whether that is a problem with the current code.

If the belief above is correct, I would suggest not changing the 'with check' versions, since their speed isn't as critical.

Comment by Gary Fredericks [ 15/Mar/14 10:23 PM ]

Thanks Andy, I'll submit a patch that changes the three non-checked methods.

Comment by Gary Fredericks [ 15/Mar/14 10:46 PM ]

Attached CLJ-1384-p1.patch, which updates the three non-check create methods.

I also added benchmarks. It's about 2-3 times faster for large collections.

Comment by Ambrose Bonnaire-Sergeant [ 11/Apr/14 11:15 AM ]

Added benchmark suite (set-bench.tar).

FWIW results are similar to gfrederick's on my machine:

Clojure 1.6

Small collections (5 elements)

set averages 1.220601 µs
into averages 1.597991 µs

Large collections (1,000,000 elements)

set averages 2.429066 sec
into averages 1.006249 sec

After transients

Small collections (5 elements)

set averages 999.248325 ns
into averages 1.162889 µs

Large collections (1,000,000 elements)

set averages 1.003792 sec
into averages 889.993185 ms

Add full output to the tar.

Comment by Ghadi Shayban [ 11/Apr/14 11:35 AM ]

CLJ-1192 is related to this, but and Andy seems to be indicating the use of reduce as the means to better performance there.

Comment by Gary Fredericks [ 11/Apr/14 11:41 AM ]

Oh that's a good point about reduce. The difference should only apply to chunked seqs, right? It's worth noting that the benchmarks above used range which creates chunked seqs, so that might be why into looks faster on the large collections?

So this change only makes set act like vec; I think whether either/both of them should use reduce is a different question.





[CLJ-1182] Regexp are never equal Created: 12/Mar/13  Updated: 11/Apr/14  Resolved: 05/Sep/13

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

Type: Enhancement Priority: Minor
Reporter: Christian Fortin Assignee: Omer Iqbal
Resolution: Declined Votes: 0
Labels: None

Attachments: File fix-CLJ-1182.diff    
Patch: Code
Approval: Triaged

 Description   

The following (= #"asd" #"asd") will return false in CLJ, even if they are the same.

Consequently, everything with a regexp in it (lists, vectors, maps...) will never be equal.

It seems to have been fixed for CLJS, but not for CLJ.
https://github.com/clojure/clojurescript/commit/e35c3a57472fa62ae41591418a73794dc8ac6dde



 Comments   
Comment by Omer Iqbal [ 12/Mar/13 4:08 PM ]

added an implementation for the equiv method if both args are java.util.regex.Pattern

Comment by Andy Fingerhut [ 12/Mar/13 5:54 PM ]

Omer, could you also include in your patch a few test cases? At least one that validates that two regex's that should be equal, are equal, and another that two regex's that should be different, are non-equal. Preferably the first of those tests should fail with the existing Clojure code, and pass with your changes.

Comment by Omer Iqbal [ 13/Mar/13 5:39 AM ]

I updated the patch with some tests. Hope I added them in the correct file. I also changed the implementation a bit, by instead of adding another implementation of equiv with a different signature, checking the type of the Object in the equiv method with signature (Object k1, Object k2).
For the sake of consistency I also added an EquivPred implementation, though I'm not entirely sure when its used.

Comment by Andy Fingerhut [ 13/Mar/13 1:04 PM ]

All comments here refer to the patch named fix-CLJ-1182.diff dated Mar 13, 2013.

The location for the new tests looks reasonable. However, note that your new patch has your old changes plus some new ones, not just the new ones. In particular, the new signature for equiv is still in your latest patch. You should start from a clean pull of the latest Clojure master and make only the changes you want when creating a patch, not build on top of previous changes you have made.

Also, there are several whitespace-only changes in your patch that should not be included.

Comment by Omer Iqbal [ 13/Mar/13 1:39 PM ]

I uploaded a clean patch, removing the whitespace diff and only adding the latest changes. Thanks for clarifying the workflow!
Just to clarify, this refers to the patch named fix-CLJ-1182.diff dated Mar 13 1:34 PM

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

I am recategorizing this as an enhancement, because if this is a bug it is a bug in Java – the Java Patterns class documents being immutable, but apparently does not implement .equals.

Other recent "make Clojure more complicated to work around problems in Java" patches have been rejected, and I suspect this one will be too, because it might impact the performance of equality everywhere.

Comment by Stuart Halloway [ 12/Apr/13 9:04 AM ]

At first pass, Rich and I both believe that, as regex equality is undecidable, that Java made the right choice in using identity for equality, that this ticket should be declined, and the ClojureScript should revert to match.

But leaving this ticket open for now so that ClojureScript dev can weigh in.

Comment by Michael Drogalis [ 12/Apr/13 9:32 AM ]

What do you mean when you say "undecidable"?

Comment by Alexander Redington [ 12/Apr/13 10:03 AM ]

If Regex instances were interned by the reader, but still used identity for equality, then code like

(= #"abc" #"abc")

would return true, but it wouldn't diverge in the definition of equality for regular expressions between Java and Clojure.

Comment by Fogus [ 12/Apr/13 10:13 AM ]

Undecidable means that for any given regular expression, there is no single way to write it. For example #"[a]" #"a" both match the same strings, but are they the same? Maybe. But how can we decide if /any/ given regular expression matches all of the same strings as any other? The answer is, you can't. Java does provide a Pattern#pattern method that returns the string that was used to build it, but I'm not sure if that could/should be used as a basis for equality given the potential perf hit.

Comment by Herwig Hochleitner [ 12/Apr/13 10:31 AM ]

I posted in Stu's thread: https://groups.google.com/d/topic/clojure-dev/OTPNJQbPtds/discussion
TL;DR: Disagree with undecidability, agree with reverting to identity based equality

Comment by Michael Drogalis [ 12/Apr/13 10:32 AM ]

That makes sense to me. Thanks Fogus.

Comment by Herwig Hochleitner [ 12/Apr/13 9:42 PM ]

From my post to the ml thread, it might not be entirely clear, why I don't think we should implement equality for host regexes:

It would involve parsing and would leave a lot of room for errors and platform-idiosycracies to leak. And it would be different for every platform.

As Alexander said, I also think this ticket could be resolved by interning regex literals, akin to keywords. That, however, would warrant a design page first, because there are tradeoffs to be made about how far the interning should go.

Comment by Rich Hickey [ 13/Apr/13 8:51 AM ]

Why are we spending time on this? Where is the problem statement? Does someone actually need this for a real world purpose not solved by using regex strings as keys?

Comment by Michael Drogalis [ 13/Apr/13 9:13 PM ]

It was prompted as a matter of consistency, which I think is valid. I can't think of a good reason to use regex's as keys though.

Comment by Trevor Hartman [ 11/Apr/14 11:13 AM ]

This issue surfaced as a bug in my code, e.g.: (get {#"^named$" 1} #"^named$") ;=> nil ; surprise!





[CLJ-1390] pprint a GregorianCalendar results in Arity exception Created: 25/Mar/14  Updated: 10/Apr/14

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

Type: Defect Priority: Minor
Reporter: Steve Suehs Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: print

Attachments: Text File 0001-CLJ-1390.patch     Text File 0002-CLJ-1390-test2.patch     Text File 0002-CLJ-1390-test.patch     Text File CLJ-1390-pprint-GregorianCalendar.patch    
Patch: Code

 Description   

What I was doing: attempting to pretty-print nested structures from Things mac app, which include instances of java.util.GregorianCalendar.
What I expected to happen: output should have an #inst very much like printing java.util.Date.
What happened instead: ArityException Wrong number of args (4) passed to: pprint$pretty-writer$fn

thingsplay.core=> (def nowish (java.util.GregorianCalendar.))
#'thingsplay.core/nowish
thingsplay.core=> nowish
#inst "2014-03-25T22:43:29.240-05:00"
thingsplay.core=> (require 'clojure.pprint)
nil
thingsplay.core=> (pprint nowish)
ArityException Wrong number of args (4) passed to: pprint$pretty-writer$fn  clojure.lang.AFn.throwArity (AFn.java:437)
#inst "
thingsplay.core=> (simple-dispatch nowish)
#inst "2014-03-25T22:43:29.240-05:00"nil
thingsplay.core=> nowish
#inst "2014-03-25T22:43:29.240-05:00"
thingsplay.core=> (write nowish)
ArityException Wrong number of args (4) passed to: pprint$pretty-writer$fn  clojure.lang.AFn.throwArity (AFn.java:437)
#inst "


 Comments   
Comment by Norman Richards [ 29/Mar/14 4:15 PM ]

The print-calendar function introduced in CLJ-928 doesn't work with clojure.pprint/pretty-writer since pretty-writer does not correctly implement the java.io.Writer interface. It only implements write(String) but print-calendar wants write(String,int,int) among others. Although pretty-writer should probably correctly implement java.io.Writer, it's pretty easy to rewrite print-calendar to use the supported subset of java.io.Writer that is implemented.

Comment by Steve Suehs [ 29/Mar/14 4:20 PM ]

Thank you, "random person at the Austin Clojure Hack Day" "that I don't know" that has a CA in place. You are awesome!

See you at the next Austin Clojure Meetup.

-s

Comment by Andy Fingerhut [ 04/Apr/14 2:51 PM ]

Norman, it would be good if the patch included a few test cases, especially ones that fail without the patch, and succeed with the patch.

Comment by Norman Richards [ 04/Apr/14 3:09 PM ]

Absolutely. I have no idea how test cases work on Clojure core, but I assume it should be easy enough to do.

Comment by Andy Fingerhut [ 04/Apr/14 3:15 PM ]

I would recommend looking at the following file in the Clojure repo, which already contains other pprint tests. It shouldn't be too difficult to get an idea from one or more of the tests there. Actually those might be slightly unusual in that many of them use a simple-tests macro defined in file test_helper.clj that most of the Clojure tests do not use, but ask questions if you have trouble, e.g. on the Clojure Google group or IRC channel.

test/clojure/test_clojure/pprint/test_pretty.clj

Comment by Steve Suehs [ 04/Apr/14 7:50 PM ]

Alright...you two are inspiring me to go work on getting my CA in.

Comment by Norman Richards [ 04/Apr/14 8:31 PM ]

Test case attached. Apply the test patch, "mvn test" fails. Apply the fix, test passes.

Comment by Andy Fingerhut [ 06/Apr/14 3:32 PM ]

It would be better if the "is" were of the form:

(is (= calculated-value "constant string to compare against") "string to show if test fails")

rather than just (is calculated-value "string to show if test fails"). The second form will fail if calculating the value throws an exception, but only the first form will calculate it, and then verify that the value is the expected one (and fail if it is not the expected one).

Comment by Norman Richards [ 07/Apr/14 10:49 AM ]

Ok - here's an alternative test case. I'm less happy with this test case, since I have to add the TimeZone and make assumptions about how the specifics of how the pretty printer formats. But, it does test the fix adequately, so if you like the test2 patch better, that's perfectly fine with me.

Comment by Steve Miner [ 10/Apr/14 4:23 PM ]

I would rather fix the actual bug in pretty_writer.clj. The proxy needs to support more of the java.io.Writer interface. I think adding another arity to the write method would work. Something like:

(write 
   ...
  ([x off len]
      (.write this (subs (str x) off (+ off len)))))
Comment by Steve Miner [ 10/Apr/14 4:38 PM ]

CLJ-1390-pprint-GregorianCalendar.patch fixes the pretty_writer.clj proxy to support the missing version of the write method. Includes the same test as the previous patch.





[CLJ-1400] Error "Can't refer to qualified var that doesn't exist" should name the bad symbol Created: 09/Apr/14  Updated: 10/Apr/14

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

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: Compiler, errormsgs
Environment:

OS X


Approval: Triaged

 Description   

Def of var with a ns that doesn't exist will yield this error:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Can't refer to qualified var that doesn't exist, compiling:(NO_SOURCE_PATH:1:1)

Cause: Compiler.lookupVar() returns null if the ns in a qualified var does not exist yet.

Proposed: The error message would be improved by naming the symbol and throwing a CompilerException with file/line/col info. It's not obvious, but this may be the only case where this error occurs. If so, the error message could be more specific that the ns is the part that doesn't exist.

Patch:

Screened by:






[CLJ-865] Macroexpansion discards &form metadata Created: 26/Oct/11  Updated: 10/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: Compiler

Attachments: Text File 0001-Add-test-for-macroexpansion-metadata-preservation.patch     Text File 0002-Preserve-form-metadata-on-macroexpanded-forms.patch     Text File 0003-Make-defmacro-preserve-form-metadata.patch     Text File 0004-Another-stab-at-implementing-this.patch     File 2013-10-11_CLJ-865_Fix-With-Tests.diff     Text File clj-865.patch     Text File clj865.patch     Text File clj-865-updated-v2-patch.txt     Text File updated.patch    
Patch: Code and Test
Approval: Vetted

 Description   

This patch changes the behavior of metadata when used in conjunction with macros. The metadata &form is now merged with the metadata of the macro call sexpr. This allows users to either type-hint the inner or the outer form in a macro call and have somewhat better results. In the past, the metadata from the macroexpand was used as-is. This disallowed code like the following, to work without reflection:

(.trim ^String (when true "hello "))

Patch: 2013-10-11_CLJ-865_Fix-With-Tests.diff
Screened by: Timothy Baldridge

--------- Implementation Details ----------

As discussed in http://groups.google.com/group/clojure/browse_thread/thread/2690cb6ca0e8beb8 there is a "surprise factor" when type-hinting an expression that represents a macro, such as with (.length ^String (doto (identity "x") prn)). Here the doto macro discards the metadata on &form, causing a reflective lookup. This has the effect that while expressions representing function calls can be type-hinted, expressions representing macros in general cannot. The doto macro could be rewritten to respect its &form metadata, but doing this for every macro in existence would be tedious and error-prone. Instead, I propose a change to the compiler, to cause macroexpansion to hang onto the metadata automatically.

The first patch attached adds a test for the behavior I propose: this test fails. After applying the second patch, the test passes.

There are a couple points that merit further consideration before accepting my patch:

  • I'm not sure I actually got the Java code formatted correctly. My editor is not well-configured to get the clojure/core style right automatically.
  • My solution is to take the &form metadata, drop :line/:file keys, and then merge with the returned metadata, with &form taking precedence. I'm not sure whether this is the right approach in all cases, even though it works for :tag metadata.
  • I achieved this with a change to the compiler, which makes it fairly heavy-weight. It should be possible to instead adjust defmacro if changes to the compiler are not desirable. However, I believe this would involve substantially more work and be harder to test (for example, multiple arities complicate things). It seems nicer to treat the macroexpansion as a black box and then make metadata tweaks to the result, rather than modifying their actual defmacro code.
  • If a macro expands to something that is not an IObj, such as an Integer, then my patch silently discards the caller's metadata. Would it be better to throw an exception?


 Comments   
Comment by Alan Malloy [ 28/Oct/11 1:12 AM ]

So I went ahead and did the work of making this change in clojure.core/defmacro instead of clojure.lang.Compiler/macroexpand1. It was even worse than I expected: I didn't realize we don't yet have syntax-quote or apply at this stage in bootstrapping, so writing a non-trivial macroexpansion requires a huge amount of (list `foo (list `bar 'local-name)) and so forth.

I'm sure the version I wrote is not optimal, but it seemed simpler to piggyback on defn, and then use alter-var-root to shim the metadata management in, than it would have been to expand to the correct thing in the first place.

Anyway, attached patch #3 could be applied instead of #2 to resolve the issue in clojure.core instead of clojure.lang. The tests added in patch #1 pass either way.

Comment by Alan Malloy [ 13/Nov/11 8:29 PM ]

I realized I can do this with a named private function instead of an anonymous function, reducing the amount of mess defmacro itself has to generate. Patch 4 is, I think, strictly better than Patch 3, if a Clojure implementation is preferred to one in Java.

Comment by Chouser [ 20/Nov/11 10:43 PM ]

I prefer patch 0002 in Java over either 0003 or 0004. Patch 0002 keeps the knowledge of how to invoke macro fns (specifically the extra &form and &env args) in one place, macroexpand1 rather than duplicating that knowledge in core.clj as well. Note patch 0001 is just tests.

The proposed default macroexpansion behavior is more useful than what we currently have, but there are two details I'd like to think about a bit more:

1) In exchange for a more useful default, macro writers lose the ability to consume their &form metadata and have control over the resulting form metadata without the &form metadata overridding it. That is, macros are no longer in complete control of their output form.

2) Rule (1) above has hardcoded exceptions for :line and :file, where &form metadata is unable to override the results returned by the macro.

Comment by Alan Malloy [ 01/Jun/12 2:04 PM ]

This patch incorporates all previous patches to this issue.

On the clj-dev mailing list, Andy Fingerhut suggested a new metadata key for allowing the macro author to specify "I've looked at their &form metadata, and this form is exactly what I want to expand to, please don't change the metadata any further." I've implemented this, and I think it addresses Chouser's concern about needing a way to "break out" of the improved-default behavior.

One open question is, is :explicit-meta the right key to use? I spent some time tracking down a bug caused by my forgetting the keyword and using :explicit-metadata in my test; perhaps something more difficult to get confused by is available.

Comment by Andy Fingerhut [ 14/Aug/13 8:05 PM ]

clj-865-updated-v2-patch.txt dated Aug 14 2013 is identical to Alan Malloy's updated.patch dated Jun 1 2012. I simply updated the patch to apply cleanly to latest master after some context lines in the test file macros.clj had gone bad due to recent commits.

Comment by Timothy Baldridge [ 11/Oct/13 9:23 AM ]

Added updated patch that works against master, and also removes COLUMN_KEY from the macro's metadata

Comment by Timothy Baldridge [ 11/Oct/13 12:50 PM ]

Added patch that contains all fixes plus a few more tests.

Comment by Rich Hickey [ 22/Nov/13 11:19 AM ]

Since this could break things, we could just take metadata on the macro name to ask for this:

(defmacro ^:keep-meta simple-macro [f arg]
  `(~f ~arg))

or something

Comment by Alan Malloy [ 03/Dec/13 1:24 AM ]

Sure, I'll put together that patch. I'm worried, though, that if it's not the default, it will just never get used, and we'll be in effectively the same situation we are now, where no macros do this right. I don't foresee anyone going through their libraries to add ^:keep-meta on every macro.

Comment by Alan Malloy [ 03/Dec/13 2:20 AM ]

I updated the patch to behave as Rich requested, but it caused a test regression that I can't figure out, in the handling of either refer or private vars. Hopefully someone else can run the tests and figure out what is missing here; my change is supposed to be opt-in, and I can't see where I've gone wrong.

Comment by Andy Fingerhut [ 07/Dec/13 10:31 AM ]

Alan, your patch clj865.patch dated Dec 3, 2013 has some HTML cruft at the beginning and end, but even after removing that it does not apply cleanly to the latest Clojure master as of today. I understand that you say it needs more work, but it would be easier for others who wish to try it out if it applied cleanly.

Comment by Alan Malloy [ 10/Dec/13 1:06 PM ]

Sorry Andy, and thanks for noticing. I haven't been on a very developer-friendly computer recently, but I'll try to fix the patch tonight.

Comment by Alan Malloy [ 11/Dec/13 10:26 AM ]

Here's a fix to the patch. I verified that this applies cleanly to current master.

Comment by Alan Malloy [ 11/Dec/13 10:27 AM ]

To clarify, it's the file named clj-865.patch. I didn't realize JIRA wouldn't make it clear which file I uploaded along with the comment.





[CLJ-1130] when unable to match a method, report arity caller was looking for Created: 17/Dec/12  Updated: 10/Apr/14

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

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Stuart Halloway
Resolution: Unresolved Votes: 0
Labels: errormsgs

Attachments: Text File clj-1130-v1.txt     File clj-1130-v2.diff     File clj-1130-v2-ignore-ws.diff     Text File clj-1130-v2.txt     File clj-1130-v3.diff     File clj-1130-v4.diff     File clj-1130-v5.diff    
Patch: Code and Test
Approval: Vetted

 Description   

Original motivation: Incorrectly invoking a static method with 0 parameters yields a NoSuchFieldException:

user=> (Long/parseLong)
CompilerException java.lang.NoSuchFieldException: parseLong, compiling:(NO_SOURCE_PATH:1:1) 
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: No matching method: parseLong, compiling:(NO_SOURCE_PATH:2:1)

Incorrectly invoking an instance method with 0 parameters yields a message about fields as well:

user=> (.setTime (java.util.Date.))
IllegalArgumentException No matching field found: setTime for class java.util.Date  clojure.lang.Reflector.getInstanceField (Reflector.java:271)

Patch: clj-1130-v5.diff

Approach: Primum non nocere. Error reporting enhanced at the site the errors happen, compiler logic unchanged.



 Comments   
Comment by Michael Drogalis [ 06/Jan/13 6:44 PM ]

It looks like it's first trying to resolve a field by name, since field access with / is legal. For example:

user=> (Integer/parseInt)
CompilerException java.lang.NoSuchFieldException: parseInt, compiling:(NO_SOURCE_PATH:1)

user=> (Integer/MAX_VALUE)
2147483647

Would trying to resolve a method before a field fix this?

Comment by Alex Miller [ 03/Sep/13 10:10 AM ]

Similarities to CLJ-1248 (there a warning, here an error).

Comment by Andy Fingerhut [ 09/Sep/13 12:36 AM ]

Patch clj-1130-v1.txt changes the error message in a situation when one attempts to invoke a static method with no args, and there is no such 0-arg static method. The message now says that there is no such method with that name and 0 args, rather than that there is no such static field with that name.

Comment by Alex Miller [ 04/Oct/13 3:56 PM ]

I updated the patch to simplify it a bit but more importantly to remove the check by exception and instead use the Reflector method that can answer this question.

Comment by Andy Fingerhut [ 12/Oct/13 3:11 PM ]

Alex, thank you for the improvements to the code. It looks better to me.

Comment by Rich Hickey [ 25/Oct/13 7:30 AM ]

due to indentation changes, this patch appears to touch much more than it probably does, making it difficult to approve.

Comment by Andy Fingerhut [ 25/Oct/13 10:59 AM ]

Any suggestions on what can be done to make progress here? Would it help to attach a patch made with "-w" option to ignore lines that differ only in whitespace? Provide git diff command line options that do this, after the patch is applied to your local workspace? Make a patch that leaves the indentation 'incorrect' after the change (involuntary shudder)?

Comment by Alex Miller [ 25/Oct/13 11:17 AM ]

The indentation has intentionally changed because the if/else structure has changed. I don't think making the patch incorrect to reduce changes is a good idea.

Comment by Andy Fingerhut [ 25/Oct/13 11:32 AM ]

Well, the 'incorrect' was in quotes because I was asking about a proposed patch that had the correct logic, but misleading indentation. Agreed it isn't a good idea, hence the shudder. I'm just brainstorming ideas to make the patch less difficult to approve.

Comment by Howard Lewis Ship [ 25/Oct/13 11:43 AM ]

At some point, you may need to bite the bullet and reformat some of the Clojure code .... Compiler.java had a crazy mix of tabs, spaces, and just completely wrong stuff.

Comment by Alex Miller [ 03/Nov/13 10:47 PM ]

Re-marking screened. Not sure what else to do.

Comment by Andy Fingerhut [ 04/Nov/13 8:35 AM ]

clj-1130-v2-ignore-ws.diff is identical to clj-1130-v2.diff, except it was produced with a command that ignores differences in a line due only to whitespace, i.e.: 'git format-patch master --stdout -w > clj-1130-v2-ignore-ws.diff'

It is not intended as the patch to be applied. It is only intended to make it easier to see that many of the lines in clj-1130-v2.diff are truly only differences in indentation.

Comment by Alex Miller [ 04/Nov/13 8:55 AM ]

Thanks Andy...

Comment by Rich Hickey [ 22/Nov/13 7:59 AM ]

This patch ignores the fact that method is checked for first above:

if(c != null)
  maybeField = Reflector.getMethods(c, 0, munge(sym.name), true).size() == 0;

Which is why the field code is unconditional. I'm fine with making errors better, but changing logic as well deserves more scrutiny.

Comment by Alex Miller [ 06/Dec/13 9:01 PM ]

This patch is intentionally trying to avoid calling StaticFieldExpr in the field code as that is where the (Long/parseLong) case (erroneously calling an n-arity static method with 0 args) will throw a field-oriented exception instead of a method-oriented exception. By adding the extra check here, this case falls through into the method case and throws later on calling StaticMethodExpr instead.

The early check is a check for methods of the specified arity. The later check is for the existence of a field of matching name. Combined, they lead to a better error message.

However, another alternative is to set maybeField in the first check based on field existence, not on invocation arity. That just improves the maybeField informaiton and the existing code then naturally throws the correct exception (and the patch is much simpler).

The similar case for calling n-arity instance methods with 0-arity has the same problem for the same reason:

user=> (.setTime (java.util.Date.))
IllegalArgumentException No matching field found: setTime for class java.util.Date  clojure.lang.Reflector.getInstanceField (Reflector.java:271)

Thus we can also adjust the other call that sets maybeField (which now is much less maybe).

I will attach a patch that covers these cases and update the ticket for someone to screen.

Comment by Stuart Sierra [ 08/Dec/13 12:24 PM ]

Screened. The patch clj-1130-v3.diff works as advertised.

This patch only improves error messages for cases when the type of the
target object is known to the compiler. In reflective calls, the error
messages are still the same.

Example, after this patch, given these definitions:

(def v 42)
(defn untagged-f [] 42)
(defn ^Long tagged-f [] 42)

The following expressions produce new error messages:

(.foo v 1)
;; IllegalArgumentException No matching method found: foo taking 1 args
;; for class java.lang.Long clojure.lang.Reflector.invokeMatchingMethod
;; (Reflector.java:53)

(.foo (tagged-f))
;; IllegalArgumentException No matching method found: foo taking 0 args
;; for class java.lang.Long clojure.lang.Reflector.invokeMatchingMethod
;; (Reflector.java:53)

These expressions still use the old error messages:

(.foo v)
;; IllegalArgumentException No matching field found: foo for class
;; java.lang.Long clojure.lang.Reflector.getInstanceField
;; (Reflector.java:271)

(.foo (untagged-f))
;; IllegalArgumentException No matching field found: foo for class
;; java.lang.Long clojure.lang.Reflector.getInstanceField
;; (Reflector.java:271)
Comment by Rich Hickey [ 03/Jan/14 8:41 AM ]

Changing the logic to get a different error message is something that needs to be done with great care. This now seems to prefer fields over methods, changing the semantics.

Comment by Stuart Halloway [ 31/Jan/14 3:12 PM ]

v4 patch simply enhances error messaages

Comment by Andy Fingerhut [ 31/Jan/14 3:18 PM ]

clj-1130-v4.diff has the same patch repeated twice in the file. clj-1130-v5.diff is identical, except deleting the redundant copy.





[CLJ-124] GC Issue 120: Determine mechanism for controlling automatic shutdown of Agents, with a default policy and mechanism for changing that policy as needed Created: 17/Jun/09  Updated: 10/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Chas Emerick Assignee: Alex Miller
Resolution: Unresolved Votes: 3
Labels: agents

Approval: Vetted
Waiting On: Rich Hickey

 Description   
Reported by cemer...@snowtide.com, Jun 01, 2009

There has been intermittent chatter over the past months from a couple of
people on the group (e.g.
http://groups.google.com/group/clojure/browse_thread/thread/409054e3542adc1f)
and in #clojure about some clojure scripts hanging, either for a constant
time (usually reported as a minute or so with no CPU util) or seemingly
forever (or until someone kills the process).

I just hit a similar situation in our compilation process, which invokes
clojure.lang.Compile from ant.  The build process for this particular
project had taken 15 second or so, but after adding a couple of pmap calls,
that build time jumped to ~1:15, with roughly zero CPU utilization over the
course of that last minute.

Adding a call to Agent.shutdown() in the finally block in
clojure.lang.Compile/main resolved the problem; a patch including this
change is attached.  I wouldn't suspect anyone would have any issues with
such a change.

-----
In general, it doesn't seem like everyone should keep tripping over this
problem in different directions.  It's a very difficult thing to debug if
you're not attuned to how clojure's concurrency primitives work under the
hood, and I would bet that newer users would be particularly affected.

After discussion in #clojure, rhickey suggested adding a
*auto-shutdown-agents* var, which:

- if true when exiting one of the main entry points (clojure.main, or the
legacy script/repl entry points), Agent.shutdown() would be called,
allowing for the clean exit of the application

- would be bound by default to true

- could be easily set to false for anyone with an advanced use-case that
requires agents to remain active after the main thread of the application
exits.

This would obviously not help anyone initializing clojure from a different
entry point, but this may represent the best compromise between
least-surprise and maximal functionality for advanced users.

------

In addition to the above, it perhaps might be worthwhile to change the
keepalive values used to create the Threadpools used by c.l.Actor's
Executors.  Currently, Actor uses a default thread pool executor, which
results in a 60s keepalive.  Lowering this to something much smaller (1s?
5s?) would additionally minimize the impact of Agent's threadpools on Java
applications that embed clojure directly (and would therefore not benefit
from *auto-shutdown-agents* as currently conceived, leading to puzzling
'hanging' behaviour).  I'm not in a position to determine what impact this
would have on performance due to thread churn, but it would at least
minimize what would be perceived as undesirable behaviour by users that are
less familiar with the implementation details of Agent and code that
depends on it.

Comment 1  by cemer...@snowtide.com, Jun 01, 2009

Just FYI, I'd be happy to provide patches for either of the suggestions mentioned
above...


 Comments   
Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/124
Attachments:
compile-agent-shutdown.patch - https://www.assembla.com/spaces/clojure/documents/a56S2ow4ur3O2PeJe5afGb/download/a56S2ow4ur3O2PeJe5afGb
124-compilation.diff - https://www.assembla.com/spaces/clojure/documents/aqn0IGxZSr3RUGeJe5aVNr/download/aqn0IGxZSr3RUGeJe5aVNr

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

oranenj said: [file:a56S2ow4ur3O2PeJe5afGb]

Comment by Assembla Importer [ 24/Aug/10 12: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 Assembla Importer [ 24/Aug/10 12:45 AM ]

cemerick said: (In [[r:fa3d24973fc415b35ae6ec8d84b61ace76bd4133]]) Add a call to Agent.shutdown() at the end of clojure.lang.Compile/main Refs #124

Signed-off-by: Chouser <chouser@n01se.net>

Branch: master

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: I'm closing this ticket to because the attached patch solves a specific problem. I agree that the idea of an auto-shutdown-agents var sounds like a positive compromise. If Rich wants a ticket to track that issue, I think it'd be best to open a new ticket (and perhaps mention this one there) rather than use this ticket to track further changes.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

scgilardi said: With both Java 5 and Java 6 on Mac OS X 10.5 Leopard I'm getting an error when compiling with this change present.

Java 1.5.0_19
Java 1.6.0_13

For example, when building clojure using "ant" from within my clone of the clojure repo:

[java] java.security.AccessControlException: access denied (java.lang.RuntimePermission modifyThread)
[java] at java.security.AccessControlContext.checkPermission(AccessControlContext.java:264)
[java] at java.security.AccessController.checkPermission(AccessController.java:427)
[java] at java.util.concurrent.ThreadPoolExecutor.shutdown(ThreadPoolExecutor.java:894)
[java] at clojure.lang.Agent.shutdown(Agent.java:34)
[java] at clojure.lang.Compile.main(Compile.java:71)

I reproduced this on two Mac OS X 10.5 machines. I'm not aware of having any enhanced security policies along these lines on my machines. The compile goes fine for me with Java 1.6.0_0 on an Ubuntu box.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: I had only tested it on my ubuntu box – looks like that was openjdk 1.6.0_0. I'll test again with sun-java5 and sun-java6.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: 1.6.0_13 worked fine for me on ubuntu, but 1.5.0_18 generated an the exception Steve pasted. Any suggestions? Should this patch be backed out until someone has a fix?

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

achimpassen said: [file:aqn0IGxZSr3RUGeJe5aVNr]

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: With Achim's patch, clojure compiles for me on ubuntu using java 1.5.0_18 from sun, and still works on 1.6.0_13 sun and 1.6.0_0 openjdk. I don't know anything about ant or the security error, but this is looking good to me.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

achimpassen said: It works for me on 1.6.0_13 and 1.5.0_19 (32 and 64 bit) on OS X 10.5.7.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: (In [[r:895b39dabc17b3fd766fdbac3b0757edb0d4b60d]]) Rev fa3d2497 causes compile to fail on some VMs – back it out. Refs #124

Branch: master

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

mikehinchey said: I got the same compile error on both 1.5.0_11 and 1.6.0_14 on Windows. Achim's patch fixes both.

See the note for "permissions" on http://ant.apache.org/manual/CoreTasks/java.html . I assume ThreadPoolExecutor.shutdown is the problem, it would shutdown the main Ant thread, so Ant disallows that. Forking avoids the permissions limitation.

In addition, since the build error still resulted in "BUILD SUCCESSFUL", I think failonerror="true" should also be added to the java call so the build would totally fail for such an error.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: I don't know if the <java fork=true> patch is a good idea or not, or if there's a better way to solve the original problem.

Chas, I'm kicking back to you, but I guess if you don't want it you can reassign to "nobody".

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

shoover said: I'd like to suggest an alternate approach. There are already well-defined and intuitive ways to block on agents and futures. Why not deprecate shutdown-agents and force users to call await and deref if they really want to block? In the pmap situation one would have to evaluate the pmap form.

The System.exit problem goes away if you configure the threadpools to use daemon threads (call new ThreadPoolExecutor and pass a thread factory that creates threads and sets daemon to true). That way the user has an explicit means of blocking and System.exit won't hang.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

alexdmiller said: I blogged about these issues at:
http://tech.puredanger.com/2010/06/08/clojure-agent-thread-pools/

I think that:

  • agent thread pool threads should be named (see ticket #378)
  • agent thread pools must be daemon threads by default
  • having ways to specify an customized executor pool for an agent send/send-off is essential to customize threading behavior
  • (shutdown-agents) should be either deprecated or made less dangerous
Comment by Alexander Taggart [ 11/Jul/11 9:33 PM ]

Rich, what is the intention behind using non-daemon threads in the agent pools?

If it is because daemon threads could terminate before their work is complete, would it be acceptable to add a shutdown hook to ensure against such premature termination? Such a shutdown hook could call Agent.shutdown(), then awaitTermination() on the pools.

Comment by Christopher Redinger [ 27/Nov/12 3:47 PM ]

Moving this ticket out of approval "OK" status, and dropping the priority. These were Assembla import defaults.

Also, Chas gets to be the Reporter now.

Comment by Chas Emerick [ 27/Nov/12 5:56 PM ]

Heh, blast from the past.

The comment import appears to have set their timestamps to the date of the import, so the conversation is pretty hard to follow, and obviously doesn't benefit from the intervening years of experience. In addition, there have been plenty of changes to agents, including some recent enhancements that address some of the pain points that Alex Miller mentioned above.

I propose closing this as 'invalid' or whatever, and opening one or more new issues to track whatever issues still persist (presumably based on fresh ML discussion, etc).

Comment by Andy Fingerhut [ 27/Nov/12 6:11 PM ]

Rereading the original description of this ticket, without reading all of the comments that follow, that description is still right on target for the behavior of latest Clojure master today.

People send messages to the Clojure Google group every couple of months hitting this issue, and one even filed CLJ-959 because of hitting it. I have updated the examples on ClojureDocs.org for future, and also for pmap and clojure.java.shell/sh which use future in their implementations, to warn people about this and explain that they should call (shutdown-agents), but making it unnecessary to call shutdown-agents would be even better, at least as the default behavior. It sounds fine to me to provide a way for experts on thread behavior to change that default behavior if they need to.





[CLJ-738] <= is incorrect when args include Double/NaN Created: 14/Feb/11  Updated: 10/Apr/14

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

Type: Defect Priority: Trivial
Reporter: Jason Wolfe Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: math
Environment:

Mac OS X, Java 6


Attachments: File 738.diff     File 738-tests.diff    
Patch: Code and Test
Approval: Vetted

 Description   
user=> (<= (long Double/NaN) 1)
true     
user=> (<= Double/NaN 1)
false  ;; should match primitive version

Cause: The problem was that the logic for lte/gte depended on the fact that lte is equivalent to !gt.
However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

Solution: The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

Patch: 738.diff, 738-tests.diff

Screened by:



 Comments   
Comment by Jason Wolfe [ 14/Feb/11 7:18 PM ]

The source of the issue seems to be incorrect treatment of boxed NaN:

user> (<= 1000 (Double. Double/NaN))
true
user> (<= 1000 (double Double/NaN))
false

Comment by Alexander Taggart [ 28/Feb/11 11:14 PM ]

Primitive comparisons use java's primitive operators directly, which always return false for NaN, even when testing equality between two NaNs.

In clojure, Number comparisons are all logical variations around calls to Numbers.Ops.lt(Number, Number). So a call to (<= x y) is actually a call to (not (< y x)), which eventually uses the primitive < operator. Alas that logical premise doesn't hold when dealing with NaN:

user=> (<= 1 Double/NaN)
false
user=> (not (< Double/NaN 1))
true

So the bug is not that boxed NaN is treated incorrectly, but rather:

user> (<= 1000 (Double. Double/NaN)) ; becomes !(NaN < 1000) 
true
user> (<= 1000 (double Double/NaN))  ; becomes (1000 <= NaN)
false

In the original example, since there are more than two args, the primitive looking args were boxed:

user=> (<= 10 Double/NaN 1) ; equivalent to logical-and of the following
true
user=> (<= 10 (Double. Double/NaN))  ; becomes !(NaN < 10)
true
user=> (<= (Double. Double/NaN) 1)   ; becomes !(1 < NaN)
true

Note however that java object comparisons for NaNs behave differently: NaN is the largest Double, and NaNs equal each other (see the javadoc).

If we make object NaN comparisons always return false, we would need to add the rest of the comparison methods to Numbers.Ops. Yet doing so could also make collection sorting algorithms behave oddly, deviating from sorting written in java. Besides, (= NaN NaN) => false is annoying.

Clojure already throws out the notion of error-free dividing by zero (which for doubles would otherwise result in NaN or Infinity, depending on the dividend). Perhaps we could similarly error on NaNs passed to clojure numeric ops. They seem to be more trouble than they're worth. That said, people smarter than me thought they were useful.

Then there's that -0.0 nonsense...

Comment by Jouni K. Seppänen [ 19/Mar/11 3:02 PM ]

On current master, (<= x y) seems to be special-cased by the compiler, but when <= is called dynamically, the bug is still there:

user=> (<= 1 Float/NaN)
false
user=> (let [op <=] (op 1 Float/NaN))
true

Since CLJ-354 got marked "Completed", perhaps there was an attempt to fix this.

Comment by Alexander Taggart [ 19/Mar/11 6:45 PM ]

Using let forces calling <= as a function rather than inlining Numbers/lte, which means the args are treated as objects not primitives, thus the different behaviour as I discussed earlier.

Comment by Aaron Bedra [ 28/Jun/11 6:28 PM ]

Rich, what should the behavior be?

Comment by Jouni K. Seppänen [ 29/Jun/11 1:22 AM ]

My suggestion for the behavior is to follow Java (Java Language Specification §15.20.1) and IEEE 754. The java.sun.com site seems to be down right now, but here's a Google cache link:

http://webcache.googleusercontent.com/search?q=cache:http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#15.20.1

Comment by Rich Hickey [ 29/Jul/11 7:48 AM ]

It should work the same as primitive double in all cases

Comment by Luke VanderHart [ 26/Aug/11 11:33 AM ]

Added patches. The problem was that our logic for lte/gte depended on the fact that lte is equivalent to !gt.

However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

Comment by Alex Miller [ 04/Mar/14 3:18 PM ]

David Welte noted: "CLJ-738 is marked Closed but the attached patch is has not been applied and both Clojure 1.5.1 and 1.6.0-beta2 exhibit the bad behavior listed in CLJ-738. The issue that CLJ-738 is that (<= (Double. Double/NaN) 1) evaluates to true while (<= Double/NaN 1) evaluates to false."

I concur that this patch was not applied. It looks likely that Luke marked this as Resolved when the patch was ready instead of whatever state change would have been appropriate at the time of the ticket (the process has varied over the years). AFAICT, this ticket should be open and Vetted (accepted as a problem) but probably needs release targeting and an updated patch based on current code.

Comment by Andy Fingerhut [ 05/Mar/14 12:32 PM ]

Might want to make the "Fix Version" on this ticket empty so it is back on the JIRA state chart as Vetted.





[CLJ-1100] Reader literals cannot contain periods Created: 02/Nov/12  Updated: 10/Apr/14

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

Type: Defect Priority: Minor
Reporter: Kevin Lynagh Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reader

Approval: Vetted

 Description   

The reader tries to read a record instead of a literal if the tag contains periods.

user> (binding [*data-readers* {'foo/bar #'identity}] (read-string "#foo/bar 1"))
1
user> (binding [*data-readers* {'foo/bar.x #'identity}] (read-string "#foo/bar.x 1"))
ClassNotFoundException foo/bar.x  java.lang.Class.forName0 (Class.java:-2)

Summary of reader forms:

Kind Example Constraint Status
Record #user.Foo[1] record class name OK
Class #java.lang.String["abc"] class name OK
Clojure reader tag #uuid "c48d7d6e-f3bb-425a-abc5-44bd014a511d" not a class name, no "/" OK
Library reader tag #my/card "5H" not a class name, has "/" OK
  #my.ns/card "5H" not a class name, has "/" OK
  #my/playing.card "5H" not a class name, has "/" BROKEN - read as record

Note: reader tags should not be allowed to override the record reader.

Cause: In LispReader, CtorReader.invoke() decides between record and tagged literal based on whether the tag has a ".".

Proposed: Change the discriminator in CtorReader.

Alternative 1 (purely string inspection):

  • If name has a "/" -> readTagged (not a legal class name)
  • If name has no "/" or "." -> readTagged (records must have qualified names)
  • Else -> readRecord (also covers Java classes)

Tradeoffs: Clojure-defined data reader tags must not contain periods. Not possible to read a Java class with no package. Avoids unnecessary class loading/construction for all tags.

Alternative 2 (prioritize Class check):

  • Attempt readRecord (also covers Java classes)
  • If failed, attempt readTagged

Tradeoffs: Clojure tags could not override Java/record constructors - I'm not sure that's something we'd ever want to do, but this would cut that off. This alternative may attempt classloading when it would not have before.

Hybrids of these are also possible.

Patch:

Screened by:



 Comments   
Comment by Steve Miner [ 06/Nov/12 9:41 AM ]

The suggested patch (clj-1100-reader-literal-periods.patch) will break reading records when *default-data-reader-fn* is set. Try adding a test like this:

(deftest tags-containing-periods-with-default
      ;; we need a predefined record for this test so we (mis)use clojure.reflect.Field for convenience
      (let [v "#clojure.reflect.Field{:name \"fake\" :type :fake :declaring-class \"Fake\" :flags nil}"]
        (binding [*default-data-reader-fn* nil]
          (is (= (read-string v) #clojure.reflect.Field{:name "fake" :type :fake :declaring-class "Fake" :flags nil})))
        (binding [*default-data-reader-fn* (fn [tag val] (assoc val :meaning 42))]
          (is (= (read-string v) #clojure.reflect.Field{:name "fake" :type :fake :declaring-class "Fake" :flags nil})))))
Comment by Rich Hickey [ 29/Nov/12 9:36 AM ]

The problem assessment is ok, but the resolution approach may not be. What happens should be based not upon what is in data-readers but whether or not the name names a class.

Is the intent here to allow readers to circumvent records? I'm not in favor of that.

Comment by Steve Miner [ 29/Nov/12 4:01 PM ]

New patch following Rich's comments. The decision to read a record is now based on the symbol containing periods and not having a namespace. Otherwise, it is considered a data reader tag. User
defined tags are required to be qualified but they may now have periods in the name. Tests added to show that
data readers cannot override record classes. Note: Clojure-defined data reader tags may be unqualified, but they should not contain periods in order to avoid confusion with record classes.

Comment by Steve Miner [ 29/Nov/12 4:17 PM ]

I deleted my old patch and some comments referring to it to avoid confusion.

In Clojure 1.5 beta 1, # followed by a qualified symbol with a period in the name is considered a record and causes an exception for the missing record class. With the patch, only non-qualified symbols containing periods are considered records. That allows user-defined qualified symbols with periods in their names to be used as data reader tags.

Comment by Andy Fingerhut [ 07/Feb/13 9:05 AM ]

clj-1100-periods-in-data-reader-tags-patch-v2.txt dated Feb 7 2013 is identical to CLJ-1100-periods-in-data-reader-tags.patch dated Nov 29 2012, except it applies cleanly to latest master. The only change appears to be in some white space in the context lines.

Comment by Andy Fingerhut [ 07/Feb/13 12:53 PM ]

I've removed clj-1100-periods-in-data-reader-tags-patch-v2.txt mentioned in the previous comment, because I learned that CLJ-1100-periods-in-data-reader-tags.patch dated Nov 29 2012 applies cleanly to latest master and passes all tests if you use this command to apply it.

% git am --keep-cr -s --ignore-whitespace < CLJ-1100-periods-in-data-reader-tags.patch

I've already updated the JIRA workflow and screening patches wiki pages to mention this --ignore-whitespace option.

Comment by Andy Fingerhut [ 13/Feb/13 11:31 AM ]

Both of the current patches, CLJ-1100-periods-in-data-reader-tags.patch dated Nov 29 2012, and clj-1100-reader-literal-periods.patch dated Nov 6 2012, fail to apply cleanly to latest master (1.5.0-RC15) as of today, although they did last week. Given all of the changes around read / read-string and edn recently, they should probably be evaluated by their authors to see how they should be updated.

Comment by Steve Miner [ 14/Feb/13 12:23 PM ]

I deleted my patch: CLJ-1100-periods-in-data-reader-tags.patch. clj-1100-reader-literal-periods.patch is clearly wrong, but the original author or an administrator has to delete that.

Comment by Kevin Lynagh [ 14/Feb/13 1:28 PM ]

I cannot figure out how to remove my attachment (clj-1100-reader-literal-periods.patch) in JIRA.

Comment by Steve Miner [ 14/Feb/13 1:43 PM ]

Downarrow (popup) menu to the right of the "Attachments" section. Choose "manager attachments".

Comment by Kevin Lynagh [ 14/Feb/13 2:02 PM ]

Great, thanks Steve. Are you going to take another pass at this issue, or should I give it a go?

Comment by Steve Miner [ 14/Feb/13 3:04 PM ]

Kevin, I'm not planning to work on this right now as 1.5 is pretty much done. It might be worthwhile discussing the issue a bit on the dev mailing list before working on a patch, but that's up to you. I think my approach was correct, although now changes would have to be applied to both LispReader and EdnReader.

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

Updated description based on my understanding.





[CLJ-1157] Classes generated by gen-class aren't loadable from remote codebase for mis-implementation of static-initializer Created: 04/Feb/13  Updated: 10/Apr/14

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

Type: Defect Priority: Minor
Reporter: Tsutomu Yano Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: gen-class
Environment:

Tested on Mac OS X 10.9.1 and Oracle JVM 1.7.0_51 with Clojure 1.6 master SNAPSHOT


Attachments: File 20140121_fix_classloader.diff    
Patch: Code
Approval: Vetted

 Description   

When a genclass'ed object is serialized and sent to a remote system, the remote system throws an exception loading the object:

Exception in thread "main" java.lang.ExceptionInInitializerError
        at java.io.ObjectStreamClass.hasStaticInitializer(Native Method)
        at java.io.ObjectStreamClass.computeDefaultSUID(ObjectStreamClass.java:1723)
        at java.io.ObjectStreamClass.access$100(ObjectStreamClass.java:69)
        at java.io.ObjectStreamClass$1.run(ObjectStreamClass.java:247)
        at java.io.ObjectStreamClass$1.run(ObjectStreamClass.java:245)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.io.ObjectStreamClass.getSerialVersionUID(ObjectStreamClass.java:244)
        at java.io.ObjectStreamClass.initNonProxy(ObjectStreamClass.java:600)
        at java.io.ObjectInputStream.readNonProxyDesc(ObjectInputStream.java:1601)
        at java.io.ObjectInputStream.readClassDesc(ObjectInputStream.java:1514)
        at java.io.ObjectInputStream.readOrdinaryObject(ObjectInputStream.java:1750)
        at java.io.ObjectInputStream.readObject0(ObjectInputStream.java:1347)
        at java.io.ObjectInputStream.readObject(ObjectInputStream.java:369)
        at sun.rmi.server.UnicastRef.unmarshalValue(UnicastRef.java:324)
        at sun.rmi.server.UnicastRef.invoke(UnicastRef.java:173)
        at java.rmi.server.RemoteObjectInvocationHandler.invokeRemoteMethod(RemoteObjectInvocationHandler.java:194)
        at java.rmi.server.RemoteObjectInvocationHandler.invoke(RemoteObjectInvocationHandler.java:148)
        at $Proxy0.getResult(Unknown Source)
        at client.SampleClient$_main.doInvoke(SampleClient.clj:12)
        at clojure.lang.RestFn.invoke(RestFn.java:397)
        at clojure.lang.AFn.applyToHelper(AFn.java:159)
        at clojure.lang.RestFn.applyTo(RestFn.java:132)
        at client.SampleClient.main(Unknown Source)
 Caused by: java.io.FileNotFoundException: Could not locate remoteserver/SampleInterfaceImpl__init.class or remoteserver/SampleInterfaceImpl.clj on classpath: 
        at clojure.lang.RT.load(RT.java:434)
        at clojure.lang.RT.load(RT.java:402)
        at clojure.core$load$fn__5039.invoke(core.clj:5520)
        at clojure.core$load.doInvoke(core.clj:5519)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at clojure.lang.Var.invoke(Var.java:415)
        at remoteserver.SampleInterfaceImpl.<clinit>(Unknown Source)
        ... 23 more

Reproduce:

// build
git clone git://github.com/tyano/clojure_genclass_fix.git
cd clojure_genclass_fix
sh build.sh
// start rmiregistry
rmiregistry -J-Djava.rmi.server.useCodebaseOnly=false &
// start server
cd remoteserver
sh start.sh
// Start client
cd ../client
sh start.sh

Cause:

A gen-classed class (in this case, SampleInterfaceImpl.class) uses a static-initializer for loading SampleInterfaceImpl__init.class like:

static
  {
    RT.var("clojure.core", "load").invoke("/remoteserver/SampleInterfaceImpl");
  }

RT.load in default uses a context-classloader for loading __init.class but all classes depending on a gen-classed class must be loaded from the same classloader. In this case, RT.load must use a remote URLClassLoader which loads the main class.

Proposed:

Instead produce the equivalent of this in the static initializer:

static
  {
    Var.pushThreadBindings(RT.map(new Object[] { Compiler.LOADER, SampleInterfaceImpl.class.getClassLoader() }));
    try {
        RT.var("clojure.core", "load").invoke("/remoteserver/SampleInterfaceImpl");
    }
    finally 
    {
        Var.popThreadBindings();
    }
  }

With this code, RT.load will uses a same classloader which load SampleInterfaceImpl.class.

Patch: 20140121_fix_classloader.diff

Screened by:



 Comments   
Comment by Stuart Halloway [ 01/Mar/13 10:20 AM ]

This sounds reasonable, but anything touching classloaders must be considered very carefully.

Comment by Stuart Halloway [ 01/Mar/13 12:12 PM ]

It seems overly complex to have the patch do so much code generation. Why not implement a method that does this job, and have the generated code call that?

Comment by Andy Fingerhut [ 11/Jan/14 2:47 PM ]

Patch 20130204_fix_classloader.diff dated Feb 3, 2013 no longer applies cleanly as of the latest commits to Clojure master on Jan 11, 2014. The only conflict in applying the patch appears to be in the file src/jvm/clojure/asm/commons/GeneratorAdapter.java. This is probably due to the commit for ticket CLJ-713 that was committed today, updating the ASM library.

Comment by Tsutomu Yano [ 21/Jan/14 3:01 AM ]

I put a new patch applicable on the latest master branch.
This new patch is simpler and robust because the code-generation becomes very simple. Now It just call a method implemented with Java.

And I fixed my sample program and the 'HOW TO REPRODUCT THIS ISSUE' section of this ticket, because old description is not runnable on newest JVM. It is because the specification of remote method call of the newest JVM was changed from the old one. In the newest JVM, we need a 'java.rmi.server.useCodebaseOnly=false' option for making the behavior of remote call same as old one.
pull the newest sample.





[CLJ-1099] better error message when passing non-seq to seq Created: 01/Nov/12  Updated: 09/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File better-error-message-for-seq.patch    
Patch: Code
Approval: Vetted

 Description   

Design discussion here.

This patch improves Clojure's error message for a single common error: passing a non-seq where a seq is neede. More importantly, it is intended as a prototype for other similar improvements in the future.

Error message before:

(cons 1 2)
=> IllegalArgumentException Don't know how to create ISeq from: java.lang.Long

Error message after:

user=> (cons 1 2)
ExceptionInfo Don't know how to create ISeq from: java.lang.Long
user=> (ex-data *e)
{:instance 2}

Patch: better-error-message-for-seq.patch
NOTE: This patch was reverted as it affected the inlining of RT.seqFrom().



 Comments   
Comment by Michael Klishin [ 12/Nov/12 10:34 AM ]

Wouldn't it be better to make it read "Don't know how to create ISeq from: 2 (java.lang.Long)"? How many beginners will figure
out ex-data exists and how to use it?

Comment by Stuart Halloway [ 12/Apr/13 11:36 AM ]

Hi Michael,

ex-info messages should not, in general, pr-str things into their bodies. This raises the question of print-length and print-level in a place where the user doesn't have good control, while the whole point of ex-info is to be in the data business, not the string business. Users can control printing from ex-data any way they like.

There are two possible ways to make beginners aware of ex-data: Tell them about it in one (or a few places) in docs, or in an infinite number of places saying "This would have been useful here, but we didn't use it because you might not know about it." I prefer the former.

That said, I think it would be great to increase the visibility of ex-info and ex-data early on in documentation for beginners, and to make sure that things like exception printing in logs are flexible enough not to lose the benefits of ex-info.

Comment by Andy Fingerhut [ 25/Mar/14 5:14 PM ]

Just a comment that this fix was committed before release 1.6.0, and then reverted very shortly before release 1.6.0. I believe the reason for reverting was due to concerns that this change made performance about 5% slower in some relatively common cases, with a suspicion that it could have affected inlining of the seqFrom method.

Not sure whether the ticket should be reopened or not.





[CLJ-992] `iterate` reducer Created: 10/May/12  Updated: 09/Apr/14

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

Type: Enhancement Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers

Attachments: Text File 0001-Add-reducers-iterate.patch     Text File iterate-reducer.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Added a reducer implementation mirroring clojure.core/iterate.

Patch: 0001-Add-reducers-iterate.patch

Screened by:



 Comments   
Comment by Alan Malloy [ 10/May/12 9:50 PM ]

Should I have made this implement Seqable as well? It wasn't clear to me, because as far as I could see this was the only function in clojure.core.reducers that's generating a brand-new sequence rather than transforming an existing one.

Comment by Alan Malloy [ 10/May/12 10:24 PM ]

Previous version neglected to include the seed value of the iteration in the reduce.

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

Currying iterate seems useless, albeit not harmful.

While implementing repeat, I couldn't use currying. Because 1-arity is already reserved for infinite repeat ([n x] and [x], not [n x] and [n] if currying)

How about we just support currying for functions where last param is reducible?

Comment by Alan Malloy [ 18/Aug/12 7:16 PM ]

This new patch replaces the previous patch. As requested, I am splitting up the large issue CLJ-993 into smaller tickets.

Does not depend on any of my other reducer patches, but there will probably be some minor merge conflicts unless it is merged after CLJ-1045 and CLJ-1046, and before CLJ-993.





[CLJ-1187] Clojure loses quoted metadata on empty literals Created: 22/Mar/13  Updated: 09/Apr/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 001-CLJ-1187.patch    
Patch: Code and Test
Approval: Vetted

 Description   

meta on empty collections is lost:

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

This bug propagates to ^:const vars:

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

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

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

Patch: 001-CLJ-1187.patch (NOTE: this change overlaps with CLJ-1093 and the patch there may be preferred)

Screened by:



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

After patch:

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

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

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

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

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

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

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

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

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

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

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

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

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

Looks good

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

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

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

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

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

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

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

Pulling out of 1.6, will consider in next release.





[CLJ-1093] Empty PersistentCollections get incorrectly evaluated as their generic clojure counterpart Created: 24/Oct/12  Updated: 09/Apr/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Timothy Baldridge
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File 0001-CLJ-1093-fix-empty-records-literal-v2.patch     Text File clj-1093-fix-empty-record-literal-patch-v2.txt    
Patch: Code and Test
Approval: Vetted

 Description   
user> (defrecord x [])
user.x
user> #user.x[]
{}
user> #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1)
clojure.lang.PersistentArrayMap

Cause: Compiler's ConstantExpr parser returns an EmptyExpr for all empty persistent collections, even if they are of types other than the core collections (for example: records, sorted collections, custom collections). EmptyExpr reports its java class as one the classes - IPersistentList/IPersistentVector/IPersistentMap/IPersistentSet rather than the original type.

Proposed: If one of the Persistent* classes, then create EmptyExpr as before, otherwise retain the ConstantExpression of the original collection.

Patch: 0001-CLJ-1093-fix-empty-records-literal-v2.patch

Screened by:



 Comments   
Comment by Timothy Baldridge [ 27/Nov/12 11:41 AM ]

Unable to reproduce this bug on latest version of master. Most likely fixed by some of the recent changes to data literal readers.

Marking Not-Approved.

Comment by Timothy Baldridge [ 27/Nov/12 11:41 AM ]

Could not reproduce in master.

Comment by Nicola Mometto [ 01/Mar/13 1:23 PM ]

I just checked, and the problem still exists for records with no arguments:

Clojure 1.6.0-master-SNAPSHOT
user=> (defrecord a [])
user.a
user=> #user.a[]
{}

Admittedly it's an edge case and I see little usage for no-arguments records, but I think it should be addressed aswell since the current behaviour is not what one would expect

Comment by Herwig Hochleitner [ 02/Mar/13 8:14 AM ]

Got the following REPL interaction:

% java -jar ~/.m2/repository/org/clojure/clojure/1.5.0/clojure-1.5.0.jar
user=> (defrecord a [])
user.a
user=> (a.)
#user.a{}
user=> #user.a{}
{}
#user.a[]
{}

This should be reopened or declined for another reason than reproducability.

Comment by Nicola Mometto [ 10/Mar/13 2:18 PM ]

I'm reopening this since the bug is still there.

Comment by Andy Fingerhut [ 13/Mar/13 2:04 PM ]

Patch clj-1093-fix-empty-record-literal-patch-v2.txt dated Mar 13, 2013 is identical to Bronsa's patch 001-fix-empty-record-literal.patch dated Oct 24, 2012, except that it applies cleanly to latest master. I'm not sure why the older patch doesn't but git doesn't like something about it.

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

Patch 0001-CLJ-1093-fix-empty-records-literal-v2.patch solves more issues than the previous patch that was not evident to me at the time.

Only collections that are either PersistentList or PersistentVector or PersistentHash[Map|Set] or PersistentArrayMap can now be EmptyExpr.
This is because we don't want every IPersistentCollection to be emitted as a generic one eg.

user=> (class #clojure.lang.PersistentTreeMap[])
clojure.lang.PersistentArrayMap

Incidentally, this patch also solves CLJ-1187
This patch should be preferred over the one on CLJ-1187 since it's more general

Comment by Jozef Wagner [ 09/Aug/13 2:08 AM ]

Maybe this is related:

user=> (def x `(quote ~(list 1 (clojure.lang.PersistentTreeMap/create (seq [1 2 3 4])))))
#'user/x
user=> x
(quote (1 {1 2, 3 4}))
user=> (class (second (second x)))
clojure.lang.PersistentTreeMap
user=> (eval x)
(1 {1 2, 3 4})
user=> (class (second (eval x)))
clojure.lang.PersistentArrayMap

Even if the collection is not evaluated, it is still converted to the generic clojure counterpart.





[CLJ-1385] Docstrings for `conj!` and `assoc!` should suggest using the return value; effect not always in-place Created: 16/Mar/14  Updated: 06/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Pyry Jahkola Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: collections, docstring

Attachments: Text File CLJ-1385-reword-docstrings-on-transient-update-funct.patch    

 Description   

The docstrings of both `assoc!` and `conj!` say "Returns coll.", suggesting the transient edit happens always in-place, `coll` being the first argument.

However, the fact that the following example omits the key `8` in its result proves that in-place edits aren't always the case:

(let [a (transient {})]
      (dotimes [x 9]
        (assoc! a x :ok))
      (persistent! a))
    ;;=> {0 :ok, 1 :ok, 2 :ok, 3 :ok, 4 :ok, 5 :ok, 6 :ok, 7 :ok}

Instead, programmers should be guided towards using constructs like `reduce` with transients:

(persistent! (reduce #(assoc! %1 %2 :ok)
                 (transient {})
                 (range 9)))
    ;;=> {0 :ok, 1 :ok, 2 :ok, 3 :ok, 4 :ok, 5 :ok, 6 :ok, 7 :ok, 8 :ok}

The easiest way to achieve this is by changing the docstrings of (at least) `conj!` and `assoc!` to not read "Returns coll." but instead tell that the change is destructive.



 Comments   
Comment by Alex Miller [ 16/Mar/14 8:49 AM ]

When modifying transient collections, it is required to use the collection returned from functions like assoc!. The ! here indicates its destructive nature. The transients page (http://clojure.org/transients) describes the calling pattern pretty explicitly: "You must capture and use the return value in the next call."

I do not agree that we should be guiding programmers away from using functions like assoc! – transients are used as a performance optimization and using assoc! or conj! in a loop is often the fastest version of that. However I do think it would be helpful to make the docstring more explicit.

Comment by Gary Fredericks [ 05/Apr/14 10:23 AM ]

Alex I think you must have misread the ticket – the OP is suggesting guiding toward using the return value of assoc!, not avoiding assoc! altogether.

And the docstring is not simply inexplicit, it's actually incorrect specifically in the case that the OP pointed out. conj! and assoc do not return coll at the point where array-maps transition to hash-maps, and the fact that they do otherwise is supposed to be an implementation detail as far as I understand it.

Comment by Alex Miller [ 05/Apr/14 11:55 AM ]

@Gary - you're right, I did misread that.

assoc and conj both explicitly say "return a new collection" whereas assoc! and conj! say "Returns coll." I read that as "returns the modified collection" without regard to whether it's the identical instance, but I can read it your way too.

Would saying "Returns updated collection." transmit the right idea? Using "collection" instead of "coll" removes the concrete tie to the variable and "updated" hints more strongly that you should use the return value.

Comment by Pyry Jahkola [ 05/Apr/14 12:47 PM ]

@Alex, that update makes it sound right to me, FWIW.

Comment by Gary Fredericks [ 05/Apr/14 2:37 PM ]

Yeah, I think that's better. Thanks Alex. I'd be happy to submit a patch for that but I'm assuming patches are too heavy for this kind of change?

Comment by Andy Fingerhut [ 06/Apr/14 3:35 PM ]

Patches are exactly what has been done in the past for this kind of change, if it is in a doc string and not on the clojure.org web page.

Comment by Alex Miller [ 06/Apr/14 4:13 PM ]

Yup, patch desired.

Comment by Gary Fredericks [ 06/Apr/14 5:32 PM ]

Glad I asked.

Patch is attached that also updates the docstring for pop! which had the same issue, though arguably it's less important since afaik pop! does always return the identical collection (but I don't think this is part of the contract).





[CLJ-1398] Update URLs in javadoc.clj Created: 02/Apr/14  Updated: 04/Apr/14

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

Type: Enhancement Priority: Trivial
Reporter: Eli Lindsey Assignee: Unassigned
Resolution: Unresolved Votes: 0
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    
Patch: Code

 Description   

Three minor fixes/enhancements to javadoc.clj:

0001 corrects the URLs for apache commons javadoc (the ones used in javadoc.clj no longer resolve).
0002 adds javadoc lookup for guava and apache commons lang3.
0003 adds javadoc lookup for jdk8.

(Note: contributor agreement is in the mail)



 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.





[CLJ-887] Error when calling primitive functions with destructuring in the arg vector Created: 29/Nov/11  Updated: 03/Apr/14

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: Alexander Taggart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Attachments: Text File 0001-don-t-remove-meta-from-arg-vector-in-maybe-destructu.patch    
Patch: Code
Approval: Triaged

 Description   

If one defines a primitive-taking function with destructuring, calling that function will result in a ClassCastException, IFF the primitive return-type hint is present.

Clojure 1.4.0-master-SNAPSHOT
user=> (defn foo [[a b] ^long x ^long y] 0)
#'user/foo
user=> (foo [1 2] 3 4)
0
user=> (defn foo ^long [[a b] ^long x ^long y] 0)
#'user/foo
user=> (foo [1 2] 3 4)
ClassCastException user$foo cannot be cast to clojure.lang.IFn$OLLL  user/eval9 (NO_SOURCE_FILE:4)
user=> (pst)
ClassCastException user$foo cannot be cast to clojure.lang.IFn$OLLL
	user/eval9 (NO_SOURCE_FILE:4)
	clojure.lang.Compiler.eval (Compiler.java:6493)
	clojure.lang.Compiler.eval (Compiler.java:6459)
	clojure.core/eval (core.clj:2796)
	clojure.main/repl/read-eval-print--5967 (main.clj:244)
	clojure.main/repl/fn--5972 (main.clj:265)
	clojure.main/repl (main.clj:265)
	clojure.main/repl-opt (main.clj:331)
	clojure.main/main (main.clj:427)
	clojure.lang.Var.invoke (Var.java:397)
	clojure.lang.Var.applyTo (Var.java:518)
	clojure.main.main (main.java:37)
nil


 Comments   
Comment by Nicola Mometto [ 03/Apr/14 1:35 PM ]

This was happening because maybe-destructured returned the arg vector without the type hint, so the function was getting compiled to a IFn$OLLO rather than a IFn$OLLL but the :arglists vector in the var meta was still tagged, so the compiler thought that foo was a IFn$OLLL.

This patch addresses this by preserving the original meta on the fn arglist

Comment by Alex Miller [ 03/Apr/14 1:40 PM ]

Weirdly, I saw this happen today in my own code.





[CLJ-1399] missing field munging when recreating deftypes serialized in to byte code Created: 02/Apr/14  Updated: 02/Apr/14

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

Type: Defect Priority: Major
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: File clj-1399.diff    
Patch: Code

 Description   

to embed deftypes in the bytecode the compiler emits the value of each field, then emits a call to the deftypes underlying class's constructor.

to get a list of fields the compiler calls .getBasis.

the getBasis fields are the "clojure" level field names of the deftype, which the actual "jvm" level field names have been munged (replacing - with _, etc), so the compiler tries to generate code to set values on non-existent fields

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L4579

https://www.refheap.com/70731

you can work around this by using field names that don't require munging. a solution might be just calling munge in the emission of field sets of ITypes



 Comments   
Comment by Kevin Downey [ 02/Apr/14 4:26 PM ]

reproducing case

$ rlwrap java -server -Xmx1G -Xms1G -jar /Users/hiredman/src/clojure/target/clojure-1.6.0-master-SNAPSHOT.jar
Clojure 1.6.0-master-SNAPSHOT
user=> (deftype Foo [hello-world])
user.Foo
user=> (alter-var-root #'default-data-readers assoc 'foo (fn [x] (Foo. x)))
{foo #<user$eval6$fn__7 user$eval6$fn__7@2f953efd>, inst #'clojure.instant/read-instant-date, uuid #'clojure.uuid/default-uuid-reader}
user=> #foo "1"
CompilerException java.lang.IllegalArgumentException: No matching field found: hello-world for class user.Foo, compiling:(NO_SOURCE_PATH:0:0)
user=>
Comment by Kevin Downey [ 02/Apr/14 4:39 PM ]

this patch fixes the issue on the latest master for me

Comment by Chas Emerick [ 02/Apr/14 4:57 PM ]

FWIW, this was precipitated by real experience (I think I created the refheap paste). The workaround is easy (don't use dashes in field names of deftypes you want to return from data reader functions), but I wouldn't expect anyone to guess that that wasn't already oversensitized to munging edge cases.





[CLJ-1394] Print multi method dispatch values in the exception messages. Created: 31/Mar/14  Updated: 01/Apr/14

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

Type: Enhancement Priority: Trivial
Reporter: Roman Scherer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

Attachments: File pr-str-dispatch-value-safe.diff    
Patch: Code and Test

 Description   

The error messages of defmulti are at the moment not as helpful as they could be under certain circumstances. Calling this multi method with a lazy seq as it's dispatch argument raises the following exception:

(defmulti test-multi identity)
(defmethod test-multi 1 [arg] true)

(test-multi (map identity [:x]))
;=> java.lang.IllegalArgumentException: No method in multimethod 'test-multi' for dispatch value: clojure.lang.LazySeq@3c6f1187

Sometimes it would be useful to actually see which values are in the lazy seq being dispatched on. A better error message could look like
this for example:

(test-multi (map identity [:x]))
;=> java.lang.IllegalArgumentException: No method in multimethod 'test-multi' for dispatch value (:x) of class clojure.lang.LazySeq

This patch addresses this issue by formatting the dispatch value via `pr-str` and printing the class before it is passed to the exception constructor. The same is also done for the methods in MultiFn.java that throw a dispatch value as part of their exception message.



 Comments   
Comment by Nicola Mometto [ 31/Mar/14 8:22 PM ]

What if the value is infinite lazy-seq?

Comment by Roman Scherer [ 01/Apr/14 2:50 AM ]

Nicola, I forgot those. But I think infinite sequences could be handled with:

(set! *print-length* 10)

I'll try it out and will update the patch later.

Any other edge cases in mind?

Comment by Roman Scherer [ 01/Apr/14 2:28 PM ]

After having read "Controlling run-away trains, onions, and exercise
bikes" [1] I now bind print-length and print-size when building
the error message. This helps when not being able to dispatch on this
for example:

(test-multi (let [x (atom 0)] (reset! x {:deeper x})))

However I'm not sure if this helps in the following case, where
dispatching would fail on an infinite seq.

(test-multi (iterate inc 0))

The above doesn't terminate in Clojure 1.6.0, nor does it when binding
print-length like the attached patch does.

[1] http://blog.n01se.net/blog-n01se-net-p-85.html





[CLJ-1396] Bad link Created: 01/Apr/14  Updated: 01/Apr/14  Resolved: 01/Apr/14

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

Type: Defect Priority: Trivial
Reporter: MG Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

http://richhickey.github.io/clojure/



 Description   

http://richhickey.github.io/clojure/
The issue tracker link points to Assembla.



 Comments   
Comment by Nicola Mometto [ 01/Apr/14 7:31 AM ]

That site is deprecated and so is the repo that's hosting it.
This is the updated one http://clojure.github.io/clojure/

Comment by Alex Miller [ 01/Apr/14 7:45 AM ]

Nicola, I agree with your assessment here but only screeners should be closing tickets, thanks.

Comment by Nicola Mometto [ 01/Apr/14 7:50 AM ]

Alex, sorry, I didn't know this, I will refrain from doing so in the future.





[CLJ-1395] get should deref delay, refs atoms etc Created: 01/Apr/14  Updated: 01/Apr/14  Resolved: 01/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Gerrit Jansen van Vuuren Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

(get (delay {:a 1}) :a)
;; nil

(get {:a 1} :a)
;; 1

The above situation can happen easily and there is no way to refactor or check that all code doing gets is in fact doing deref before doing get. Given that everything is dynamic typing, changing a single value from a map to delay or ref can make large parts of the code fail silently on nil for gets.

get would be more consistent (with what is expected of it) if it was to check for refs, atoms and delays and do a deref.



 Comments   
Comment by Alex Miller [ 01/Apr/14 7:43 AM ]

I think this goes beyond what get should do. In regards to silent failure, that is covered by CLJ-1107 which would cause this to throw an exception instead.





[CLJ-1397] exception testing broken over map Created: 01/Apr/14  Updated: 01/Apr/14  Resolved: 01/Apr/14

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

Type: Defect Priority: Major
Reporter: MG Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: test
Environment:

Linux ... 3.2.0-56-generic-pae #86-Ubuntu SMP ... i686 i686 i386 GNU/Linux



 Description   

Expected: Tests pass
Actual: Two tests fail
To reproduce, run the following test file:

(ns pe.test-test
(:require [clojure.test :refer :all]))
(defn throwexc [m] (throw (Exception. m)))
(defn throwass [m] (assert false m))
(defn nestexc [] (throwexc "exc"))
(defn nestass [] (throwass "ass"))
(defn nestmapexc [] (map throwexc '("a" "b" "c")))
(defn nestmapass [] (map throwass '("a" "b" "c")))
(deftest exceptions-and-assertions-test
(testing "throwing"
(is (thrown? Exception (throwexc "exc")))
(is (thrown? AssertionError (throwass "ass"))))
(testing "nesting"
(is (thrown? Exception (nestexc)))
(is (thrown? AssertionError (nestass))))
(testing "nesting over map"
(is (thrown? Exception (nestmapexc)))
(is (thrown? AssertionError (nestmapass)))))



 Comments   
Comment by MG [ 01/Apr/14 7:25 AM ]

Clarification: The two assertions in "nesting over map" fails, the other four assertions succeed.

Comment by Nicola Mometto [ 01/Apr/14 7:30 AM ]

map is lazy, the exception is never thrown because the sequence is never realized.
either wrap the map in a dorun or use a doseq instead of a map.
map should not be used for side-effecting





[CLJ-1059] PersistentQueue doesn't implement java.util.List, causing nontransitive equality Created: 03/Sep/12  Updated: 31/Mar/14

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

Type: Defect Priority: Major
Reporter: Philip Potter Assignee: Philip Potter
Resolution: Unresolved Votes: 1
Labels: queue

Attachments: File 001-clj-1059-make-persistentqueue-implement-list.diff     File 002-clj-1059-asequential-rebased-to-cached-hasheq.diff    
Patch: Code and Test

 Description   

PersistentQueue implements Sequential but doesn't implement java.util.List. Lists form an equality partition, as do Sequentials. This means that you can end up with nontransitive equality:

(def q (conj clojure.lang.PersistentQueue/EMPTY 1 2 3))
;=> #user/q
(def al (doto (java.util.ArrayList.) (.add 1) (.add 2) (.add 3)))
;=> #user/al
(def v [1 2 3])
;=> #user/v
(= al v)
;=> true
(= v q)
;=> true
(not= al q)
;=> true

This happens because PersistentQueue is a Sequential but not a List, ArrayList is a List but not a Sequential, and PersistentVector is both.



 Comments   
Comment by Philip Potter [ 15/Sep/12 3:41 AM ]

Whoops, according to http://dev.clojure.org/display/design/JIRA+workflow I should have emailed clojure-dev before filing this ticket. Here is the discussion:

https://groups.google.com/d/topic/clojure-dev/ME3-Ke-RbNk/discussion

Comment by Philip Potter [ 15/Sep/12 2:37 PM ]

Attached 001-make-PersistentQueue-implement-List.diff, 15/Sep/12

Note that this patch has a minor conflict with the one I added to CLJ-1070, because both add an extra interface to PersistentQueue - List in this case, IHashEq in CLJ-1070.

Comment by Chouser [ 18/Sep/12 1:04 AM ]

Philip, patch looks pretty good – thanks for doing this. A couple notes:

This is only my opinion, but I prefer imports be listed without wildcards, even if it means an extra couple lines at the top of a .java file.

I noticed the "List stuff" code is a copy of what's in ASeq and EmptyList. I suppose this is copied because EmptyList and PersistentQueue extend Obj and therefore can't extend ASeq. Is this the only reason? It seems a shame to duplicate these method definitions, but I don't know of a better solution, do you?

It would also be nice if the test check a couple of the List methods you've implemented.

Comment by Chouser [ 18/Sep/12 1:08 AM ]

oh, also "git am" refused to apply the patch, but I'm not sure why. "patch -p 1" worked perfectly.

Comment by Philip Potter [ 18/Sep/12 1:19 AM ]

did you use the --keep-cr option to git am?

I struggled to know whether I should be adding CRs or not to line endings, because the files I was editing weren't consistent in their usage. If you open them in emacs, half the lines have ^M at the end.

Comment by Philip Potter [ 18/Sep/12 1:21 AM ]

Will submit another patch, with the import changed. I'll have a think about the list implementation and see what ideas I can come up with.

Comment by Philip Potter [ 18/Sep/12 3:17 PM ]

Attached 002-make-PersistentQueue-implement-Asequential.diff

This patch is an alternative to 001-make-PersistentQueue-implement-List.diff

So I took on board what you said about ASeq, but it didn't feel right making PersistentQueue directly implement ISeq, somehow.

So I split ASeq into two parts – ASequential, which implements j.u.{Collection,List} and manages List-equality and hashcodes; and ASeq, which... doesn't seem to be doing much anymore, to be honest.

As a bonus, this patch fixes CLJ-1070 too, so I went and added the tests from that ticket in to demonstrate this fact. It also tidies up PersistentQueue by removing all equals/hashcCode stuff and all Collection stuff.

(It turns out that because ASeq was already implementing Obj, the fact that PersistentQueue was implementing Obj was no barrier to using it.)

Would appreciate comments on this approach, and how it differs from the previous patch here and the patch on CLJ-1070.

Comment by Philip Potter [ 18/Sep/12 3:44 PM ]

Looking at EmptyList's implementation of List, it is a duplicate of the others, but it shouldn't be. I think its implementation of indexOf is the biggest culprit - it should just be 'return -1;' but it has a great big for loop! But this is beyond the scope of this ticket, so I won't patch that here.

Comment by Andy Fingerhut [ 20/Oct/12 12:29 PM ]

Philip, now that the patch for CLJ-1070 has been applied, these patches no longer apply cleanly. Would you be willing to update them? If so, please remove the obsolete patches.

Comment by Philip Potter [ 22/Oct/12 5:10 AM ]

Andy, thanks so much for your efforts to make people aware of these things. I will indeed submit new patches, hopefully later this week.

Comment by Philip Potter [ 03/Nov/12 12:23 PM ]

Replaced existing patches with new ones which apply cleanly to master.

There are two patches:

001-clj-1059-make-persistentqueue-implement-list.diff

This fixes equality by making PersistentQueue implement List directly. I also took the opportunity to remove the wildcard import and to add tests for the List methods, as compared with the previous version of the patch.

002-clj-1059-asequential.diff

This fixes equality by creating a new abstract class ASequential, and making PersistentQueue extend this.

My preferred solution is still the ASequential patch, but I'm leaving both here for comparison.

Comment by Timothy Baldridge [ 30/Nov/12 3:37 PM ]

Vetting.

Comment by Andy Fingerhut [ 11/Dec/12 12:50 PM ]

Philip, this time I think it was patches that were committed for CLJ-1000 that make your patch 002-clj-1059-asequential.diff not apply cleanly. I often fix up stale patches where the change is straightforward and mechanical, but in this case you are moving some methods that CLJ-1000's patch changed the implementation of, so it would be best if someone figured out a way to update this patch in a way that doesn't clobber the CLJ-1000 changes.

Comment by Philip Potter [ 11/Dec/12 1:57 PM ]

Thanks Andy. Submitted a new patch, 002-clj-1059-asequential-rebased-to-cached-hasheq.diff, which supersedes 002-clj-1059-asequential.diff.

The patch 001-clj-1059-make-persistentqueue-implement-list.diff still applies cleanly, and is still an alternative to 002-clj-1059-asequential-rebased-to-cached-hasheq.diff.

Comment by Andy Fingerhut [ 30/Jan/14 4:50 PM ]

With the commits to Clojure master made in the week leading up to Jan 30 2014, particularly changes to hasheq, patch 002-clj-1059-asequential-rebased-to-cached-hasheq.diff no longer applies cleanly.

Patch 001-clj-1059-make-persistentqueue-implement-list.diff still does.

Comment by Andy Fingerhut [ 31/Mar/14 5:33 PM ]

This issue was run into again and a duplicate ticket CLJ-1374 created – later closed as a duplicate of this one. Just wanted to record that this issue is being hit by others besides those originally reporting it.





[CLJ-1374] Make PersistentQueue implement List Created: 09/Mar/14  Updated: 31/Mar/14  Resolved: 31/Mar/14

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

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

Attachments: File clj-1374-1.diff    
Patch: Code and Test

 Description   

Most ordered Clojure collections like lists, vectors, and lazy seqs implement java.util.List, and thus .equals can be true between values of those types and other collections implementing java.util.List, like java.util.Vector and java.util.ArrayList.

Clojure PersistentQueue seems to be the odd man out here, in that it implements Collection but not List, and thus while it can be .equals to Clojure lists, vectors, and lazy seqs, it cannot be .equals to other collections implementing java.util.List.

user=> (instance? java.util.List '())
true
user=> (instance? java.util.List (lazy-seq))
true
user=> (instance? java.util.List [])
true
user=> (instance? java.util.List (vector-of :long))
true
user=> (instance? java.util.List clojure.lang.PersistentQueue/EMPTY)
false

user=> (= '() (java.util.ArrayList.))
true
user=> (= (lazy-seq) (java.util.ArrayList.))
true
user=> (= [] (java.util.ArrayList.))
true
user=> (= (vector-of :long) (java.util.ArrayList.))
true
user=> (= clojure.lang.PersistentQueue/EMPTY (java.util.ArrayList.))
false


 Comments   
Comment by Andy Fingerhut [ 09/Mar/14 6:29 PM ]

Patch clj-1374-1.diff is written assuming that CLJ-1372 patch clj-1372-2.diff or very similar has been committed, because of the tests modified, not because of the change of PersistentQueue to implement the java.util.List interface. I can update this patch as desired if that change does not go in.

Comment by Andy Fingerhut [ 09/Mar/14 6:33 PM ]

Ugh. The subject is definitely a duplicate of CLJ-1059, which I should have checked for before creating this ticket. I will compare patches to see how the approaches compare. Mine is probably a poor substitute for that one, but the tests I add may still be useful to keep in a patch for CLJ-1059.

Comment by Andy Fingerhut [ 31/Mar/14 5:29 PM ]

Problem description is a duplicate of CLJ-1059. Even the patch (independently developed) is nearly the same as the patch with a name beginning with "001" attached to CLJ-1059.





[CLJ-1241] NPE when AOTing overrided clojure.core functions Created: 30/Jul/13  Updated: 31/Mar/14

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

Type: Defect Priority: Minor
Reporter: Phil Hagelberg Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: aot, compiler

Attachments: Text File 0001-fix-CLJ-1241.patch    
Patch: Code
Approval: Triaged

 Description   

When performing AOT compilation on a namespace that overrides a clojure.core function without excluding the original clojure.core function from the ns, you get a NullPointerException.

To reproduce:

$ lein new aot-get
$ cd aot-get
$ sed -i s/foo/get/
$ lein compile :all
WARNING: get already refers to: #'clojure.core/get in namespace: aot-get.core, being replaced by: #'aot-get.core/get
Exception in thread "main" java.lang.NullPointerException
	at clojure.lang.Compiler$ObjExpr.emitVar(Compiler.java:4858)
	at clojure.lang.Compiler$DefExpr.emit(Compiler.java:428)
	at clojure.lang.Compiler.compile1(Compiler.java:7152)
	at clojure.lang.Compiler.compile(Compiler.java:7219)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)

Cause: DefExpr.parse does not call registerVar for vars overridding clojure.core ones, thus when AOT compiling the var is not registered in the constant table.

Proposed: The attached patch makes DefExpr.parse call registerVar for vars overridding clojure.core ones.

Patch: 0001-fix-CLJ-1241.patch



 Comments   
Comment by Nicola Mometto [ 30/Jul/13 7:29 PM ]

DefExpr.parse was not calling registerVar for vars overridding clojure.core ones.

Comment by Alex Miller [ 31/Jul/13 12:25 AM ]

Verified on Clojure 1.5.1.

Comment by Javier Neira Sanchez [ 27/Aug/13 8:34 AM ]

Reproduced with `key` function without `(:refer-clojure :exclude [key])`

Comment by Rich Hickey [ 05/Sep/13 8:32 AM ]

This doesn't meet triage guidelines - i.e. there is this problem, therefore we will fix it by _____ so it then does _____

Comment by Aaron Cohen [ 26/Mar/14 12:52 PM ]

This is still present in the 1.6 release. I think it's mis-classified as low priority.

Comment by Aaron Cohen [ 26/Mar/14 12:52 PM ]

See for instance the cascalog mailing list: https://groups.google.com/forum/#!topic/cascalog-user/Pe5QIpmU0vA

Comment by Andy Fingerhut [ 26/Mar/14 1:07 PM ]

It may help if someone could clarify Rich's comment.

Does it mean that the ticket should include a plan of the form "therefore we will fix it by _____ so it then does _____", but this ticket doesn't have that?

Or perhaps it means that the ticket should not include a plan of that form, but this ticket does? If so, I don't see it, except perhaps the very last sentence of the description. If that is a problem for vetting a ticket, perhaps we could just delete that sentence and proceed from there?

Something else?

Comment by Nicola Mometto [ 26/Mar/14 1:13 PM ]

Andy, I added the two last lines in the description after reading Rich's comment to explain why this bug happens and how the patch I attached works around this.

I don't know if this is what he was asking for though.

Comment by Alex Miller [ 27/Mar/14 11:00 AM ]

I think Rich meant that a ticket should have a plan of that form but does not. My own take on "triaged" is that it should state actual and expected results demonstrating a problem - I don't think it needs to actually describe the solution (as that can happen later in development). It is entirely possible that Rich and I differ in our interpretation of that. I will see if I can rework the description a bit to match what I've been doing elsewhere.

Comment by Andy Fingerhut [ 31/Mar/14 9:34 AM ]

Alex, I have looked through the existing wiki pages on the ticket tracking process, and do not recall seeing anything about this desired aspect of a triaged ticket. Is it already documented somewhere and I missed it? Not that it has to be documented, necessarily, but Rich saying "triage guidelines" makes it sound like a filter he applies that ticket creators and screeners maybe should know about.

Comment by Alex Miller [ 31/Mar/14 11:57 AM ]

To me, Triage (and Vetting) is all about having good problem statements. For a defect, it is most important to demonstrate the problem (what happens now) and what you expect to happen instead. I do not usually expect there to necessarily be "by ____" in the ticket - to me that is part of working through the solution (although it is typical to have this in an enhancement). This ticket, as it stands now, seems to have both a good problem statement and a good cause/solution statement so seems to exceed Triaging standards afaik.

Two places where I have tried to write about these things in the past are http://dev.clojure.org/display/community/Creating+Tickets and in the Triage process on the workflow page http://dev.clojure.org/display/community/JIRA+workflow.





[CLJ-979] map->R returns different class when invoked from AOT code Created: 03/May/12  Updated: 29/Mar/14

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

Type: Defect Priority: Major
Reporter: Edmund Jackson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: aot
Environment:

Mac OS X 10.5, lein 1.7 and lein 2.0


Attachments: Text File clj-979-symptoms.patch    

 Description   

Compiling a class via `deftype` during AOT compilation gives different results for the different constructors. These hashes should be identical.

user=> (binding [*compile-files* true] (eval '(deftype Abc [])))
user.Abc
user=> (hash Abc)
16446700
user=> (hash (class (->Abc)))
31966239


 Comments   
Comment by Scott Lowe [ 12/May/12 9:05 PM ]

I can't reproduce this under Clojure 1.3 or 1.4, and Leiningen 1.7.1 on either Java 1.7.0-jdk7u4-b21 OpenJDK 64-Bit or Java 1.6.0_31 Java HotSpot 64-Bit. OS is Mac OS X 10.7.

Edmund, how are you running this AOT code? I wrapped your code in a main function and built an uberjar from it.

Comment by Edmund Jackson [ 13/May/12 2:20 AM ]

Hi Scott,

Interesting.

I have two use cases
1. AOT compile and call from repl.
My steps: git clone, lein compile, lein repl, (use 'aots.death), (in-ns 'aots.death), (= (class (Dontwork. nil)) (class (map->Dontwork {:a 1}))) => false

2. My original use case, which I've minimised here, is an AOT ns, producing a genclass that is called instantiated from other Java (no main). This produces the same error. I will produce an example of this and post it too.

Comment by Edmund Jackson [ 13/May/12 4:23 AM ]

Hi Scott,

Here is an example of it failing in the interop case: https://github.com/ejackson/aotquestion2
The steps I'm following to compile this all up are

git clone git@github.com:ejackson/aotquestion2.git
cd aotquestion2/cljside/
lein uberjar
lein install
cd ../javaside/
mvn package
java -jar ./target/aotquestion-1.0-SNAPSHOT.jar

and it dies with this:

Exception in thread "main" java.lang.ClassCastException: cljside.core.Dontwork cannot be cast to cljside.core.Dontwork
at cljside.MyClass.makeDontwork(Unknown Source)
at aotquestion.App.main(App.java:8)

The error message is really confusing (to me, anyway), but I think its the same root problem as for the REPL case.

What do you see when you run the above ?

Comment by Scott Lowe [ 13/May/12 8:41 AM ]

Ah, yes, looks like my initial attempt to reproduce was too simplistic. I used your second git repo, and can now confirm that it's failing for me with the same error.

Comment by Scott Lowe [ 13/May/12 10:35 PM ]

I looked into this a little further and the AOT generated code looks correct, in the sense that both code paths appear to be returning the same type.

However, I wonder if this is really a ClassLoader issue, whereby two definitions of the same class are being loaded at different times, because that would cause the x.y.Class cannot be cast to x.y.Class exception that we're seeing here.

Comment by Steve Miner [ 03/Sep/13 9:54 AM ]

This could be related to CLJ-1157 which deals with a ClassLoader issue with AOT compiled code.

Comment by Ambrose Bonnaire-Sergeant [ 29/Mar/14 1:11 PM ]

I've tried this patch attached to CLJ-1157 and it did not solve this issue.

Comment by Ambrose Bonnaire-Sergeant [ 29/Mar/14 2:27 PM ]

This bug seems to be rooted in different behaviour for do/let under compilation. Attached a patch showing these symptoms in the hope it helps people find the cause.





[CLJ-1117] partition docstring should be more explicit about dropped or partial trailing elements Created: 29/Nov/12  Updated: 28/Mar/14

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: 4
Labels: docstring
Environment:

OS X, 10.8


Attachments: Text File clj-1117.patch    
Patch: Code
Approval: Triaged

 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: The docstring should be updated to make it clear that not providing a pad means that items are dropped, and to also see partition-all.

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





[CLJ-1297] try to catch using - instead of _ in filenames so the compiler can give a better error message for people who don't know that you need to use _ in file names Created: 19/Nov/13  Updated: 28/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 10
Labels: compiler, errormsgs

Attachments: File better-error-messages-for-require.diff    
Patch: Code and Test
Approval: Triaged

 Description   

Problem: Clojure requires the files that back a namespace that has dashes in it to have the dashes replaced with underscores on the filesystem (ie a.b_c.clj for namespace a.b-c). If you require a file that has been mistakenly saved as b-c.clj instead, you will get an error message:

Exception in thread "main" java.io.FileNotFoundException: Could not locate a/b_c__init.class or a/b_c.clj on classpath:
	at clojure.lang.RT.load(RT.java:443)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5018.invoke(core.clj:5530)
	at clojure.core$load.doInvoke(core.clj:5529)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5336)
	at clojure.core$load_lib$fn__4967.invoke(core.clj:5375)
	at clojure.core$load_lib.doInvoke(core.clj:5374)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:619)
	at clojure.core$load_libs.doInvoke(core.clj:5413)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:619)
	at clojure.core$require.doInvoke(core.clj:5496)

Proposed:

  • When loading the resource-root of lib throws a FileNotFoundException, the lib is analyzed...
  • ... if the lib was a name that would be munged, it examines the combinatorial explosion of munge candidates and .clj or .class files in the classpath ...
  • ... if any of these candidates exist, it informs the user of the file's existence, and that a change to that filename would lead to that resource being loaded.
  • ... if none of these candidates exist, it throws the original exception.

It also modifies clojure.lang.RT to expose the behavior around finding clj or class files from a resource root.

Patch: better-error-messages-for-require.diff



 Comments   
Comment by Joshua Ballanco [ 20/Nov/13 12:15 AM ]

A perhaps even better solution would be to simply allow the use of dashes in *.clj[s] filenames. I can't imagine the extra disk access per-namespace would be a huge performance burden, and (since dashes aren't allowed currently) I don't think there would be any issues with backwards compatibility.

Comment by Gary Fredericks [ 20/Nov/13 8:40 AM ]

It's worth mentioning the combinatorial explosion for namespaces with multiple dashes – if I (require 'foo-bar.baz-bang), should clojure search for all four possible filenames? Does the jvm have a way to search for files by regex or similar to avoid nasty degenerate cases (like (require 'foo-------------))?

Comment by Joshua Ballanco [ 20/Nov/13 11:08 AM ]

According to the docs, the FileSystem class's "getPathMatcher" method accepts path globs, so you'd merely have to replace each instance of "-" or "_" with "{-,_}". Actual runtime characteristics would likely depend on the underlying filesystem's implementation.

Comment by Alex Miller [ 20/Nov/13 12:02 PM ]

I don't think the FileSystem stuff applies when looking up classes on the classpath. Note that Java class names cannot contain "-".

Comment by Phil Hagelberg [ 21/Nov/13 12:05 PM ]

According to the spec, Java class names can't contain dashes (though IIRC OpenJDK and Oracle's JDK accept them anyway) but the requirement that Clojure source files have names which align with their AOT'd class file eqivalents is something we've imposed upon ourselves. Introducing the disconnect between .clj files and .class files makes way more sense than disconnecting namespaces and .clj files, but arguably it's too late to fix that mistake.

In any case a check for dashed files (resulting only in a more informative compiler error, not a more permissive compiler) which only triggers when a .clj file cannot be found imposes zero overhead in the case where things are already working.

Comment by scott tudd [ 09/Dec/13 2:19 PM ]

As Clojure seems to be idiomatic to have sometimes-dashed-namespace-and-function-names as opposed to the ubiquitous camelCaseFunctionNames in java ... I agree to have the compiler automagically handle 'knowing' to look in dir_struct AND dir-struct for requisite files.

or at the least print out a nice message explaining the quirk when files "can't" be found ... WHEN there are dashes and underscores involved... anything to aid in helping things "just work" as one would think they're supposed to.

Comment by Obadz [ 12/Dec/13 5:28 AM ]

I would have saved a few hours as well.

Comment by Alexander Redington [ 14/Feb/14 2:29 PM ]

This patch changes clojure.core/load such that:

  • When loading the resource-root of lib throws a FileNotFoundException, the lib is analyzed...
  • ... if the lib was a name that would be munged, it examines the combinatorial explosion of munge candidates and .clj or .class files in the classpath ...
  • ... if any of these candidates exist, it informs the user of the file's existance, and that a change to that filename would lead to that resource being loaded.
  • ... if none of these candidates exist, it throws the original exception.

It also modifies clojure.lang.RT to expose the behavior around finding clj or class files from a resource root.

Comment by Andy Fingerhut [ 20/Mar/14 1:16 PM ]

I do not know whether it handles all of the cases proposed in this discussion, but I encourage folks to check out the filename/namespace consistency checking in the latest Eastwood release (version 0.1.1) to see if it catches the cases they would hope to catch. It does a static check based on the files in a Leiningen project, nothing at run time. https://github.com/jonase/eastwood

Of course changes to Clojure itself to give warnings about such things can still be very useful, since not everyone will be using a 3rd party tool to check for such things.





[CLJ-1393] clojure.string/trim doesn't trim null character Created: 28/Mar/14  Updated: 28/Mar/14  Resolved: 28/Mar/14

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

Type: Defect Priority: Minor
Reporter: Ryan Fowler Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: string


 Description   

CLJ-935 changed clojure.string/trim to not trim all the characters less than or equal to \u0020 as Java does.

I noticed this because base64 uses null characters to pad the end of encoding blocks.

Clojure 1.6.0's trim leaves the null character in:
user=> (.length (clojure.string/trim "\u0000"))
1

java.lang.String's trim takes it out:
user=> (.length (.trim "\u0000"))
0

Here are the first 21 unicode characters and what Character/isWhitespace says about them.

(dotimes [n 0x20] (printf "
u%04x - %b\n" n (Character/isWhitespace n)))
\u0000 - false
\u0001 - false
\u0002 - false
\u0003 - false
\u0004 - false
\u0005 - false
\u0006 - false
\u0007 - false
\u0008 - false
\u0009 - true
\u000a - true
\u000b - true
\u000c - true
\u000d - true
\u000e - false
\u000f - false
\u0010 - false
\u0011 - false
\u0012 - false
\u0013 - false
\u0014 - false
\u0015 - false
\u0016 - false
\u0017 - false
\u0018 - false
\u0019 - false
\u001a - false
\u001b - false
\u001c - true
\u001d - true
\u001e - true
\u001f - true



 Comments   
Comment by Alex Miller [ 28/Mar/14 12:27 PM ]

The choice was made in CLJ-935 to consistently define whitespace as Character.isWhitespace() across trim, triml, and trimr. There are many possible ways to define "space" (at least two as we see here). If your trimming needs differ from the standard library, then you'll probably need to define your own functions to trim your data. You can still use Java interop to call String.trim() directly if that happens to match your needs.

Comment by Ryan Fowler [ 28/Mar/14 1:03 PM ]

Indeed, it's an easy workaround to use Java interop once you figure out what your problem is.

It's just unintuitive that the character generally used for string termination isn't trimmed by clojure.string/trim.





[CLJ-1392] AOT vs "var already exists warning" results in NPE Created: 26/Mar/14  Updated: 26/Mar/14  Resolved: 26/Mar/14

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

Type: Defect Priority: Major
Reporter: Aaron Cohen Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: aot, compiler


 Description   

I just saw this thread on the cascalog list: https://groups.google.com/forum/#!topic/cascalog-user/Pe5QIpmU0vA

Apparently the "WARNING: some? already refers to: #'clojure.core/some? in namespace: jackknife.seq, being replaced by: #'jackknife.seq/some?" results in a NullPointerException when trying to AOT a namespace that results in the message being output.

Reproducer:

1) lein new aotFail
2) Edit project.clj
;add as appropriate
:aot :all
:dependencies [[org.clojure/clojure "1.6.0"]
[cascalog "2.0.0"]]
2) Add "(:use cascalog.api)" to the ns block of src/aotFail/core.clj
3) lein compile

Output:

Compiling aotFail.core
WARNING: some? already refers to: #'clojure.core/some? in namespace: jackknife.seq, being replaced by: #'jackknife.seq/some?
Exception in thread "main" java.lang.NullPointerException, compiling:(api.clj:1:1)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3558)
at clojure.lang.Compiler.compile1(Compiler.java:7226)
at clojure.lang.Compiler.compile1(Compiler.java:7216)
at clojure.lang.Compiler.compile(Compiler.java:7292)
at clojure.lang.RT.compile(RT.java:398)
at clojure.lang.RT.load(RT.java:438)
at clojure.lang.RT.load(RT.java:411)
at clojure.core$load$fn__5066.invoke(core.clj:5641)
at clojure.core$load.doInvoke(core.clj:5640)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.core$load_one.invoke(core.clj:5446)
at clojure.core$load_lib$fn__5015.invoke(core.clj:5486)
at clojure.core$load_lib.doInvoke(core.clj:5485)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:626)
at clojure.core$load_libs.doInvoke(core.clj:5528)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:628)
at clojure.core$use.doInvoke(core.clj:5618)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at cascalog2.core$loading_4958auto_.invoke(core.clj:1)
at clojure.lang.AFn.applyToHelper(AFn.java:152)
at clojure.lang.AFn.applyTo(AFn.java:144)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3553)
at clojure.lang.Compiler.compile1(Compiler.java:7226)
at clojure.lang.Compiler.compile1(Compiler.java:7216)
at clojure.lang.Compiler.compile(Compiler.java:7292)
at clojure.lang.RT.compile(RT.java:398)
at clojure.lang.RT.load(RT.java:438)
at clojure.lang.RT.load(RT.java:411)
at clojure.core$load$fn__5066.invoke(core.clj:5641)
at clojure.core$load.doInvoke(core.clj:5640)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.core$load_one.invoke(core.clj:5446)
at clojure.core$compile$fn__5071.invoke(core.clj:5652)
at clojure.core$compile.invoke(core.clj:5651)
at user$eval19.invoke(form-init2092370125048380878.clj:1)
at clojure.lang.Compiler.eval(Compiler.java:6703)
at clojure.lang.Compiler.eval(Compiler.java:6693)
at clojure.lang.Compiler.load(Compiler.java:7130)
at clojure.lang.Compiler.loadFile(Compiler.java:7086)
at clojure.main$load_script.invoke(main.clj:274)
at clojure.main$init_opt.invoke(main.clj:279)
at clojure.main$initialize.invoke(main.clj:307)
at clojure.main$null_opt.invoke(main.clj:342)
at clojure.main$main.doInvoke(main.clj:420)
at clojure.lang.RestFn.invoke(RestFn.java:421)
at clojure.lang.Var.invoke(Var.java:383)
at clojure.lang.AFn.applyToHelper(AFn.java:156)
at clojure.lang.Var.applyTo(Var.java:700)
at clojure.main.main(main.java:37)
Caused by: java.lang.NullPointerException
at clojure.lang.Compiler$ObjExpr.emitVar(Compiler.java:4944)
at clojure.lang.Compiler$DefExpr.emit(Compiler.java:437)
at clojure.lang.Compiler.compile1(Compiler.java:7225)
at clojure.lang.Compiler.compile(Compiler.java:7292)
at clojure.lang.RT.compile(RT.java:398)
at clojure.lang.RT.load(RT.java:438)
at clojure.lang.RT.load(RT.java:411)
at clojure.core$load$fn__5066.invoke(core.clj:5641)
at clojure.core$load.doInvoke(core.clj:5640)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.core$load_one.invoke(core.clj:5446)
at clojure.core$load_lib$fn__5015.invoke(core.clj:5486)
at clojure.core$load_lib.doInvoke(core.clj:5485)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:626)
at clojure.core$load_libs.doInvoke(core.clj:5524)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:628)
at clojure.core$use.doInvoke(core.clj:5618)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at cascalog.api$loading_4958auto_.invoke(api.clj:1)
at clojure.lang.AFn.applyToHelper(AFn.java:152)
at clojure.lang.AFn.applyTo(AFn.java:144)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3553)



 Comments   
Comment by Nicola Mometto [ 26/Mar/14 12:42 PM ]

Duplicate of http://dev.clojure.org/jira/browse/CLJ-1241





[CLJ-1107] 'get' should throw exception on non-Associative argument Created: 13/Nov/12  Updated: 26/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 7
Labels: None

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.



 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.





[CLJ-1391] Allow logical operators on assert expressions Created: 26/Mar/14  Updated: 26/Mar/14

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

Type: Defect Priority: Minor
Reporter: Sanel Zukan Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: clojure.test


 Description   

With current code, it is not possible to express logical operators on some clojure.test assert expressions. For example, this will work:

(is (thrown? Exception <some-expression>))

however, here will fail:

(is (not (thrown? Exception <some-expression>)))

since '(thrown?)' is not an ordinary function, but looks like. This also adds confusion which is hard to explain to others unless '(is)' code was shown first.

Also, if the one would like to implement macro (e.g. 'is-not-thrown?') in form:

(defmacro is-not-thrown? [e expr]
  `(is (not ('thrown? ~e ~expr))))

which could be even more confusing for a person not knowing how 'thrown?' is implemented.






[CLJ-1380] Three-arg ExceptionInfo constructor permits nil data Created: 13/Mar/14  Updated: 25/Mar/14

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

Type: Defect Priority: Minor
Reporter: Gordon Syme Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File clj-1380.patch    
Patch: Code and Test

 Description   

The argument check in the two-arg clojure.lang.ExceptionInfo constructor isn't present in the three-arg constructor so it's possible to create an ExceptionInfo with arbitrary (or nil) data.

E.g.:

user=> (clojure-version)
"1.5.1"

user=> (ex-info "hi" nil)
IllegalArgumentException Additional data must be a persistent map: null  clojure.lang.ExceptionInfo.<init> (ExceptionInfo.java:26)

user=> (ex-info "hi" nil (Throwable.))
NullPointerException   clojure.lang.ExceptionInfo.toString (ExceptionInfo.java:40)


 Comments   
Comment by Gordon Syme [ 13/Mar/14 10:47 AM ]

Sorry, didn't meant to classify as "major" and I don't have permissions to edit.

Comment by Gordon Syme [ 13/Mar/14 11:11 AM ]

Patch + tests

I'm not at all familiar with the project so may have put tests in the wrong language and/or wrong place.

The ex-info-works test is a bit dorky but shows that both constructors are equivalent (and passes without the patch to ExceptionInfo).

Comment by Alex Miller [ 13/Mar/14 12:18 PM ]

No worries on the classification - I adjust most incoming tickets in some way or another.

Thanks for the patch, however it cannot be considered unless you complete the Clojure Contributor's Agreement - http://clojure.org/contributing. This is an important step in the process that keeps the Clojure codebase on a sound legal basis.

Someone else could develop a clean room patch implementation for this ticket later, but of course it would be ideal if you could become a contributor!

Comment by Gordon Syme [ 13/Mar/14 1:15 PM ]

Hi Alex,

sure, that makes sense. I'll get the contributor's agreement in the post. It may take a while to arrive since I'm based in Europe.

Comment by Gordon Syme [ 25/Mar/14 10:03 AM ]

I just checked http://clojure.org/contributing, looks like my CCA made it through





[CLJ-373] update-in with empty key paths Created: 04/Jun/10  Updated: 24/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: None

Attachments: Text File 0001-CLJ-373-Alter-behaviour-of-update-in-with-empty-key-.patch     Text File 0001-Support-empty-path-in-update-in.-CLJ-373.patch     Text File clj-373-alter-behavior-of-update-in-with-empty-key-patch2.txt     Text File CLJ-373-nested-ops.patch    
Patch: Code and Test
Approval: Triaged
Waiting On: Chouser

 Description   

To the topic of get-in and update-in. While I realize this is not a bug it is odd and in my eyes unexpected and unwanted behavior.

get-in called with an empty path returns the hashmap it was given to walk through - this is as I expect and makes a lot of sense when dynamically (with generated pathes ) walking a hash map.

update-in behaves differently and while from the implementation side, it's behavior too makes sense, it does not work as expected (at least not for me) update-in with an empty map creates a new key 'nil' so:

(update-in {...} [] f) ist he same as (update-in {...} [nil] f) while (get-in {...} []) is not the same as (get-in {...} [nil]) and of cause differs from what update-in does.

For automatically walking trees the behavior of get-in makes a lot more sense since the current behavior of update-in forces you to check for empty paths and if they are empty fall back to plain assoc and get (or get-in since this works):

(if-let [r (butlast @path)]
(do
  (alter m update-in r dissoc (last @path))
  (alter m update-in r assoc {:name @sr} c))
(do
  (alter m dissoc (last @path))
  (alter m assoc {:name @sr} c)))

Next argument is that update-in with an empty map working on nil isn't easy to gasp, one needs to know the implementation details to realize that it works, I think 90% of the people reading update-in with [] will not instinctively know that it works on the key nil, so changing this would most likely not break any current code, and if it would the code would be bad anyway .

Chouser has, a very nice solution on the mailing list that would fix the problem I'm not sure if I'm entitled to post it here since I did not wrote it but it can be found in this thread: http://groups.google.com/group/clojure/browse_thread/thread/de5b20b8c3fe498b?hl=en

Regards,
Heinz



 Comments   
Comment by Assembla Importer [ 08/Oct/10 10:33 AM ]

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

Comment by Chouser [ 13/Nov/10 6:37 PM ]

I've attached my code from the g.group thread in the form of a patch, in case it's sufficient. (Thanks to abedra for the gentle kick in the pants.)

Comment by Stuart Halloway [ 29/Nov/10 8:20 AM ]

Looks to me like the patch is misusing if-let, e.g.

(when-let [[k & mk] []] "Doh!") 
=> Doh!

Please correct, and add tests for nil and [] keys (at least).

Comment by Scott Lowe [ 08/May/12 12:20 PM ]

I will write some tests and correct this.

Comment by Scott Lowe [ 09/May/12 8:39 PM ]

I'm sorry to report that my good intentions of wanting to help clear some of the project backlog has created more work by way of further questions. I'd also like to clarify the desired new behaviours for the test cases.

Heinz proposed that an empty key sequence will not create a new nil key in the returned map.
He also suggested that the following behaviour changes be made (compare old and new* behaviours):

 
(update-in {1 2} [] (constantly {2 3}))
{nil {2 3}, 1 2}

(update-in* {1 2} [] (constantly {2 3}))
{2 3}

(update-in {1 2} [] assoc  1 3)
{nil {1 3}, 1 2}

(update-in* {1 2} [] assoc  1 3)
{1 3}

Chouser also added that nil keys should be honoured, as before:

 
(update-in* {nil 2} [nil] (constantly 3))
{nil 3}

I've added a variety of tests to cover the existing behaviour and would like to confirm that the above is all that's required for new behaviour.

The patch from November 2010 didn't work, but I tweaked it with a when-let as Stuart suggested and placed a check for an empty sequence of keys before the when-let block; because essentially, the primary behaviour change boils down to simply handling an empty sequence of keys, in addition to the existing behaviours.

I'm not entirely convinced that these changes are a good thing, but at least there's now something concrete for discussion. Please have a look at what is there. The good news is that at least there are some tests covering update-in now.

Comment by Andy Fingerhut [ 24/May/12 10:35 PM ]

clj-373-alter-behavior-of-update-in-with-empty-key-patch2.txt dated May 24, 2012 supersedes patch 0001-CLJ-373Alter-behaviour-of-update-in-with-empty-key.patch dated May 9, 2012. It makes no changes to the contents of the patch except in the lines of context that have changed since the earlier patch was created. It applies cleanly to the latest master whereas the earlier patch no longer does. Builds and passes tests with Oracle/Apple JDK 1.6 on Mac OS X 10.6.8.

Comment by Fogus [ 15/Aug/12 11:21 AM ]

This patch creates a new error mode highlighted by the following:

(update-in {:a 1} [] inc)

; ClassCastException clojure.lang.PersistentArrayMap cannot be cast to java.lang.Number

The reason is that the code that executes in the event that the keys are empty (or nil) blindly assumes that the function given can be applied to the map itself. This is no problem in the case of assoc and the like, but clearly not for inc and many other useful functions.

The old behavior was to throw an NPE and if any code relies on that fact is now broken. Maybe this is not a problem, but I'm kicking it back to get a little more discussion and to request that whatever the ultimate fix happens to be, its behavioral changes and error modes are noted in the docstring for update-in.

Comment by Gunnar Völkel [ 08/Sep/12 6:11 AM ]

I vote for changing `update-in` to support empty key vectors.

Because I think "(apply f m args)" in case of an empty vector is the natural continuation of the usual behavior of update-in:

  • update-in with 2 keys the second level map is updated
  • update-in with 1 key the first level map (child of given map) is updated
  • update-in with no key the given map (zero level) is updated.

Otherwise, you will always have to wrap update-in in something like the following when the keys vector is computed:

(if (seq keys) (apply update-in m keys f args) (apply f m args))

To Fogus' last post: (update-in {:a {:b 1}} [:a] inc) fails similar and is not handled with a special exception.

Comment by Tim McCormack [ 25/Nov/13 11:45 AM ]

Fogus, if there was code relying on that brokenness, I'd say it was only working by accident.

Comment by Ambrose Bonnaire-Sergeant [ 05/Jan/14 9:13 AM ]

Attached new patch CLJ-373-nested-ops.patch.

It implements update-in with an empty path equivalent to (apply f m args).

Since they are clearly related, I changed assoc-in to throw an error on an empty path, and updated {update,get,assoc}-in's docstring to reflect these changes.

I changed the terminology of a "sequence of keys" to a "path of keys (a sequence)", and henceforth referred to the sequence as a key path, or the name of the related arg.

Comment by Andy Fingerhut [ 14/Feb/14 11:42 AM ]

Patch CLJ-373-nested-ops.patch dated Jan 5 2014 no longer applies cleanly to latest Clojure master as of Feb 14 2014. It did on Feb 7 2014. I haven't checked in detail, but this is probably simply due to some tests recently added to a test file that require updating some diff context lines.

Comment by Ambrose Bonnaire-Sergeant [ 24/Mar/14 12:39 PM ]

Updated with clean patch.





[CLJ-1386] Add transient? predicate Created: 17/Mar/14  Updated: 23/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Devin Walters Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, transient
Environment:

N/A


Attachments: Text File 0001-Add-transient-predicate.patch     Text File 0002-Add-transient-predicate.patch    
Patch: Code and Test
Approval: Triaged

 Description   

I've encountered situations where I wanted to check whether something was transient in order to know whether I should call assoc! or assoc, conj! or conj, etc.

This patch adds `transient?` as a predicate fn.



 Comments   
Comment by Alex Miller [ 17/Mar/14 10:21 AM ]

Patch needs a docstring and a test.

Comment by Devin Walters [ 17/Mar/14 4:42 PM ]

Alex: I figured that would be the case! Sorry about that. I've updated the patch. It now includes a docstring and has tests of `transient?` for #{}, [], and {}.

Thanks!

Comment by Alex Miller [ 17/Mar/14 9:48 PM ]

Thanks - please don't use the labels "patch" or "test" - those are covered by the Patch field.

Comment by Devin Walters [ 18/Mar/14 9:17 AM ]

Ah, sorry for the mixup Alex. I assumed you removed "patch" as a label the first time around to flag this ticket as still needing a vetted patch. My mistake.

Comment by Andy Fingerhut [ 21/Mar/14 1:42 PM ]

Patch 0001-Add-transient-predicate.patch dated Mar 17, 2014 applies cleanly to latest Clojure master, but fails a test because the new function transient? has no :added metadata. See most other Clojure functions in clojure.core for examples.

It also generates a new warning while running tests:

WARNING: transient? already refers to: #'clojure.core/transient? in namespace: clojure.test-clojure.data-structures, being replaced by: #'clojure.test-clojure.data-structures/transient?

There is an older (but equivalent) definition of transient? in test file data_structures.clj that should be removed when adding it to clojure.core

Comment by Devin Walters [ 22/Mar/14 11:29 PM ]

@Andy, the reason I did not add :added metadata is because I do not know if/when this patch will be accepted, and as a result, I don't really know if it will sneak into 1.6.X or 1.7. For now, I've put it in as 1.7. If it's in the running to be added sooner than that, let me know and I'll adjust it.

RE: The warning. Good catch. I've submitted a new patch which removes the private version of transient? from data_structures.clj. All tests pass.

Edit to Add: The latest patch as of this comment is now 0002-Add-transient-predicate.patch.





[CLJ-1323] AsmReflector throws exceptions on JDK8 Created: 13/Jan/14  Updated: 23/Mar/14

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

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

Attachments: File clj-1323-disable.diff    

 Description   

After the commit of the updated ASM library for CLJ-713, Clojure builds and passes all tests except for one, compare-reflect-and-asm in reflect.clj.

This can be narrowed down somewhat to a difference in behavior of the following 2 forms evaluated with the latest Clojure and JDK8:

;; The following two lines work with the latest (Jan 11 2014) Clojure 1.6.0-master-SNAPSHOT
;; if run on JDK 6 or JDK 7, but throw an exception with JDK 8.

(import '[clojure.asm ClassReader ClassVisitor Type Opcodes])
(def r (ClassReader. "java.lang.Object"))

I am not certain, but from a bit of Google searching it appears that this may be a limitation of the ASM library version 4 – it throws exceptions when attempting to read class files produced by JDK 8, because of a newer classfile version number. Links that seem to support this conclusion:

http://mail-archive.ow2.org/asm/2013-02/msg00000.html

http://forge.ow2.org/tracker/index.php?func=detail&aid=316375&group_id=23&atid=350023

A couple of alternatives are:

(1) update ASM again to one that supports JDK 8 class files

(2) disable the compare-reflect-and-asm test. Clojure itself does not use the AsmReflector for anything except this unit test. The Java reflector is the default one.



 Comments   
Comment by Alex Miller [ 13/Jan/14 8:16 AM ]

1) There is no released ASM that supports JDK 8 yet. ASM 5 will but it will not be final till JDK 8 is in the final stages of release.

2) Probably more likely.

Comment by Nicola Mometto [ 19/Mar/14 9:01 AM ]

As of now, both JDK8 and ASM5 are out.
I just tried compiling clojure on JDK8 with ASM5 and all compiles fine

Comment by Alex Miller [ 19/Mar/14 9:07 AM ]

How are you running this test?

Comment by Nicola Mometto [ 19/Mar/14 9:11 AM ]

I downloades ASM5, replaced the bundled ASM that comes with clojure with that one after changing che package name to "clojure.asm" and run `mvn install`, all the tests pass.

Comment by Alex Miller [ 19/Mar/14 9:24 AM ]

I was actually talking about the JDK 8 change - was curious about exactly what was being changed?

Comment by Alex Miller [ 19/Mar/14 10:04 AM ]

In particular, I'm assuming that you're not altering the build.xml to change the compilation -source or -target and running with JAVA_HOME / path set to JDK 8.

We don't have any plans to actually build Clojure with JDK 8 any time soon, so I'm not overly concerned about that. But it does appear that the embedded ASM 4 cannot read newer class files from JDK 8. Afaik, the only place that happens is in clojure.reflect.java in the AsmReflector, which is not the default reflector. The JavaReflector will properly reflect the Java 8 classes.

ASM 5 has only been out a couple days and already has at least one serious bug reported - I'd like that to see more use before we switch to it, so maybe this is a good target for the release after Clojure 1.6.

Comment by Alex Miller [ 23/Mar/14 12:17 PM ]

Patch to temporarily disable the failing test until we have an ASM that supports JDK 8.





[CLJ-1387] reduce-kv on hash map ignores reduced objects in large maps Created: 18/Mar/14  Updated: 22/Mar/14  Resolved: 22/Mar/14

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

Type: Defect Priority: Major
Reporter: Jozef Wagner Assignee: Jozef Wagner
Resolution: Completed Votes: 0
Labels: None
Environment:

JRE 7


Attachments: File clj-1387.diff     File clj-1387-v2.diff     File clj-1387-v3.diff    
Patch: Code and Test
Approval: Ok

 Description   

Larger hash maps have nested INodes. As kvreduce implementations in INodes dereference reduced objects, parent INodes continue to reduce.

user=> (defn test-reduce-kv [m] (reduce-kv (fn [_ k v] (when (== 1 k) (reduced :foo))) nil m))
#'user/test-reduce-kv
user=> (test-reduce-kv (zipmap (range 3) (range 3)))
:foo
user=> (test-reduce-kv (zipmap (range 300) (range 300)))
nil

Dereferencing reduced objects should happen only PersistentHashMap/kvreduce - intermediate nodes should pass the Reduced object along.

Patch: clj-1387-v3.diff
Screened-by:



 Comments   
Comment by Alex Miller [ 18/Mar/14 5:11 PM ]

I updated the patch to use a generative test that will try many combinations of map size and the reduced index to bail out on. This test failed before applying the source patch and passes with it.

Comment by Rich Hickey [ 21/Mar/14 7:33 AM ]

if(root != null){ - return root.kvreduce(f,init); + init = root.kvreduce(f,init); + if(RT.isReduced(init)) + return ((IDeref)init).deref(); }

Turns code that always had a return into code that sometimes does.

Comment by Alex Miller [ 21/Mar/14 9:07 AM ]

Added new version of patch that retains the return flow and doesn't fall through.





[CLJ-691] missing stacktrace confuses REPL error reporting Created: 13/Dec/10  Updated: 21/Mar/14  Resolved: 17/Dec/10

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

Type: Defect Priority: Major
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0691-stacktrace-missing.patch    
Approval: Ok

 Description   

when Java is in a bad mood, exceptions start flying without any accompanying stack traces. This causes the default REPL's error reporting to barf, as it currently assumes that the first stack trace element always exists. You can see this by trying the following snippet at the REPL several times in succession:

(def x (byte-array 100000000))

When Java is somewhat angry, you will see the appropriate

OutOfMemoryError Java heap space  clojure.lang.Numbers.byte_array (Numbers.java:1409)

Eventually Java gets angrier and stops providing stack traces, causing an aget error in clojure.main instead.



 Comments   
Comment by Stuart Halloway [ 13/Dec/10 9:46 AM ]

Results with attached patch:

(def x (byte-array 100000000))
=> OutOfMemoryError Java heap space  clojure.lang.Numbers.byte_array (Numbers.java:1409)
;; eventually you get:
def x (byte-array 100000000))
=>OutOfMemoryError Java heap space  [trace missing]
Comment by Ivan Kozik [ 21/Mar/14 3:33 AM ]

To spare readers who find this page after Googling "trace missing": don't go on a wild goose chase for an OpenJDK bug that makes stack traces disappear. It's a somewhat-documented HotSpot optimization, as described here: https://github.com/technomancy/leiningen/issues/1025#issuecomment-38253962

Comment by Andy Fingerhut [ 21/Mar/14 1:49 PM ]

See also CLJ-1102 which fixes some other cases where Clojure was throwing new exceptions when incorrectly expecting at least 1 StackTraceElement in an exception's stack trace. That problem existed up through Clojure 1.5.1, but is fixed in Clojure 1.6.0.





[CLJ-1373] LazySeq should utilize cached hash from its underlying seq. Created: 09/Mar/14  Updated: 20/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, performance
Environment:

1.6.0 master SNAPSHOT


Attachments: File clj-1373.diff    
Patch: Code
Approval: Triaged

 Description   

Even if underlying seq contains a cached hash, LazySeq computes it every time.

user=> (def a (range 100000))
#'user/a
user=> (time (hash a))
"Elapsed time: 46.904273 msecs"
375952610
user=> (time (hash a)) ;; RECOMPUTE
"Elapsed time: 10.879098 msecs"
375952610
user=> (def b (seq a))
#'user/b
user=> (time (hash b))
"Elapsed time: 10.572522 msecs"
375952610
user=> (time (hash b)) ;; CACHED HASH
"Elapsed time: 0.024927 msecs"
375952610
user=> (def c (lazy-seq b))
#'user/c
user=> (time (hash c))
"Elapsed time: 12.207651 msecs"
375952610
user=> (time (hash c))
"Elapsed time: 10.995798 msecs"
375952610


 Comments   
Comment by Jozef Wagner [ 09/Mar/14 9:20 AM ]

Added patch which checks if underlying seq implements IHashEq and if yes, uses that hash instead of recomputing.





[CLJ-1250] Reducer (and folder) instances hold onto the head of seqs Created: 03/Sep/13  Updated: 20/Mar/14

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

Type: Defect Priority: Critical
Reporter: Christophe Grand Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: memory, reducers

Attachments: Text File after-change.txt     Text File before-change.txt     Text File CLJ-1250-20131211.patch     Text File CLJ-1250-AllInvokeSites-20140204.patch     Text File CLJ-1250-AllInvokeSites-20140320.patch    
Patch: Code
Approval: Triaged

 Description   

Problem Statement
A shared function #'clojure.core.reducers/reducer holds on to the head of a reducible collection, causing it to blow up when the collection is a lazy sequence.

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

Reproduction steps:
Compare the following calls:

(time (reduce + 0 (map identity (range 1e8))))
(time (reduce + 0 (r/map identity (range 1e8))))

The second call should fail on a normal or small heap.

(If reducers are faster than seqs, increase the range.)

Approaches:

1) Reimplement the #'reducer (and #'folder) transformation fns similar to the manner that Christophe proposes here:

(defrecord Reducer [coll xf])

(extend-protocol 
  clojure.core.protocols/CollReduce
  Reducer
      (coll-reduce [r f1]
                   (clojure.core.protocols/coll-reduce r f1 (f1)))
      (coll-reduce [r f1 init]
                   (clojure.core.protocols/coll-reduce (:coll r) ((:xf r) f1) init)))

(def rreducer ->Reducer) 

(defn rmap [f coll]
  (rreducer coll (fn [g] 
                   (fn [acc x]
                     (g acc (f x))))))

Advantages: Relatively non-invasive change.
Disadvantages: Not evident code. Additional protocol dispatch, though only incurred once

2) Clear the reference to 'this' on the stack just before a tail call occurs

When a callee is in return position, clear the local variable reference to 'this' in the caller's stack frame, which will make the caller and all its closed-overs eligible for reclamation.

Patch 1211 takes this approach for InvokeExpr call sites in return position. Patch 1214 takes the same approach for InvokeExprs and also static and instance interop calls.

Here is the code that performs the clearing excerpted from the 1214 patch:

void emitClearThis(GeneratorAdapter gen) {
		gen.visitInsn(Opcodes.ACONST_NULL);
		gen.visitVarInsn(Opcodes.ASTORE, 0);
	}

Tail calls wrapped inside a try/catch/finally clause cannot have 'this' cleared, because closed-overs/locals may need to be emitted for exception handling blocks. Both patches consider and handle this edge case.

Advantages: Fixes this case with no user code changes. Enables GC to do reclaim closed-overs references earlier.
Disadvantages: A compiler change.

3) Alternate approach

from Christophe Grand:
Another way would be to enhance the local clearing mechanism to also clear "this" but it's complex since it may be needed to keep a reference to "this" for caches long after obvious references to "this" are needed.

Advantages: Fine-grained
Disadvantages: Complex, invasive, and the compiler is hard to hack on.

Mitigations
Avoid reducing on lazy-seqs and instead operate on vectors / maps, or custom reifiers of CollReduce or CollFold. This could be easier with some implementations of common collection functions being available (like iterate and partition).

See https://groups.google.com/d/msg/clojure-dev/t6NhGnYNH1A/2lXghJS5HywJ for previous discussion.



 Comments   
Comment by Gary Fredericks [ 03/Sep/13 8:53 AM ]

Fixed indentation in description.

Comment by Ghadi Shayban [ 11/Dec/13 11:08 PM ]

Adding a patch that clears "this" before tail calls. Verified that Christophe's repro case is fixed.

Will upload a diff of the bytecode soon.

Any reason this juicy bug was taken off 1.6?

Comment by Ghadi Shayban [ 11/Dec/13 11:17 PM ]

Here's the bytecode for the clojure.core.reducers/reducer reify before and after the change... Of course a straight diff isn't useful because all the line numbers changed. Kudos to Gary Trakhman for the no.disassemble lein plugin.

Comment by Christophe Grand [ 12/Dec/13 6:58 AM ]

Ghadi, I'm a bit surprised by this part of the patch: was the local clearing always a no-op here?

-		if(context == C.RETURN)
+		if(shouldClear)
 			{
-			ObjMethod method = (ObjMethod) METHOD.deref();
-			method.emitClearLocals(gen);
+                            gen.visitInsn(Opcodes.ACONST_NULL);
+                            gen.visitVarInsn(Opcodes.ASTORE, 0);
 			}

The problem with this approach (clear this on tail call) is that it adds yet another special case. To me the complexity stem from having to keep this around even if the user code doesn't refer to it.

Comment by Ghadi Shayban [ 12/Dec/13 7:19 AM ]

Thank you - I failed to mention this in the commit message: it appears that emitClearLocals() belonging to both ObjMethod and FnMethod (its child) are empty no-ops. I believe the actual local clearing is on line 4855.

I agree re: another special case in the compiler.

Comment by Alex Miller [ 12/Dec/13 8:56 AM ]

Ghadi re 1.6 - this ticket was never in the 1.6 list, it has not yet been vetted by Rich but is ready to do so when we open up again after 1.6.

Comment by Ghadi Shayban [ 12/Dec/13 8:59 AM ]

Sorry I confused the critical list with the Rel1.6 list.

Comment by Ghadi Shayban [ 14/Dec/13 11:16 AM ]

New patch 20131214 that handles all tail invoke sites (InvokeExpr + StaticMethodExpr + InstanceMethodExpr). 'StaticInvokeExpr' seems like an old remnant that had no active code path, so that was left as-is.

The approach taken is still the same as the original small patch that addressed only InvokeExpr, except that it is now using a couple small helpers. The commit message has more details.

Also a 'try' block with no catch or finally clause now becomes a BodyExpr. Arguably a user error, historically accepted, and still accepted, but now they are a regular BodyExpr, instead of being wrapped by a the no-op try/catch mechanism. This second commit can be optionally discarded.

With this patch on my machine (4/8 core/thread Ivy Bridge) running on bare clojure.main:
Christophe's test cases both run i 3060ms on a artificially constrained 100M max heap, indicating a dominant GC overhead. (But they now both work!)

When max heap is at a comfortable 2G the reducers version outpaces the lazyseq at 2100ms vs 2600ms!

Comment by Ghadi Shayban [ 13/Jan/14 10:48 AM ]

Updating stale patch after latest changes to master. Latest is CLJ-1250-AllInvokeSites-20140113

Comment by Ghadi Shayban [ 04/Feb/14 3:50 PM ]

Updating patch after murmur changes

Comment by Tassilo Horn [ 13/Feb/14 4:52 AM ]

Ghadi, I suffer from the problem of this issue. Therefore, I've applied your patch CLJ-1250-AllInvokeSites-20140204.patch to the current git master. However, then I get lots of "java.lang.NoSuchFieldError: array" errors when the clojure tests are run:

     [java] clojure.test-clojure.clojure-set
     [java] 
     [java] java.lang.NoSuchFieldError: array
     [java] 	at clojure.core.protocols$fn__6026.invoke(protocols.clj:123)
     [java] 	at clojure.core.protocols$fn__5994$G__5989__6003.invoke(protocols.clj:19)
     [java] 	at clojure.core.protocols$fn__6023.invoke(protocols.clj:147)
     [java] 	at clojure.core.protocols$fn__5994$G__5989__6003.invoke(protocols.clj:19)
     [java] 	at clojure.core.protocols$seq_reduce.invoke(protocols.clj:31)
     [java] 	at clojure.core.protocols$fn__6017.invoke(protocols.clj:48)
     [java] 	at clojure.core.protocols$fn__5968$G__5963__5981.invoke(protocols.clj:13)
     [java] 	at clojure.core$reduce.invoke(core.clj:6213)
     [java] 	at clojure.set$difference.doInvoke(set.clj:61)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:442)
     [java] 	at clojure.test_clojure.clojure_set$fn__1050$fn__1083.invoke(clojure_set.clj:109)
     [java] 	at clojure.test_clojure.clojure_set$fn__1050.invoke(clojure_set.clj:109)
     [java] 	at clojure.test$test_var$fn__7123.invoke(test.clj:704)
     [java] 	at clojure.test$test_var.invoke(test.clj:704)
     [java] 	at clojure.test$test_vars$fn__7145$fn__7150.invoke(test.clj:721)
     [java] 	at clojure.test$default_fixture.invoke(test.clj:674)
     [java] 	at clojure.test$test_vars$fn__7145.invoke(test.clj:721)
     [java] 	at clojure.test$default_fixture.invoke(test.clj:674)
     [java] 	at clojure.test$test_vars.invoke(test.clj:718)
     [java] 	at clojure.test$test_all_vars.invoke(test.clj:727)
     [java] 	at clojure.test$test_ns.invoke(test.clj:746)
     [java] 	at clojure.core$map$fn__2665.invoke(core.clj:2515)
     [java] 	at clojure.lang.LazySeq.sval(LazySeq.java:40)
     [java] 	at clojure.lang.LazySeq.seq(LazySeq.java:49)
     [java] 	at clojure.lang.Cons.next(Cons.java:39)
     [java] 	at clojure.lang.RT.boundedLength(RT.java:1655)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:130)
     [java] 	at clojure.core$apply.invoke(core.clj:619)
     [java] 	at clojure.test$run_tests.doInvoke(test.clj:761)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:137)
     [java] 	at clojure.core$apply.invoke(core.clj:617)
     [java] 	at clojure.test.generative.runner$run_all_tests$fn__527.invoke(runner.clj:255)
     [java] 	at clojure.test.generative.runner$run_all_tests$run_with_counts__519$fn__523.invoke(runner.clj:251)
     [java] 	at clojure.test.generative.runner$run_all_tests$run_with_counts__519.invoke(runner.clj:251)
     [java] 	at clojure.test.generative.runner$run_all_tests.invoke(runner.clj:253)
     [java] 	at clojure.test.generative.runner$test_dirs.doInvoke(runner.clj:304)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:137)
     [java] 	at clojure.core$apply.invoke(core.clj:617)
     [java] 	at clojure.test.generative.runner$_main.doInvoke(runner.clj:312)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
     [java] 	at user$eval564.invoke(run_tests.clj:3)
     [java] 	at clojure.lang.Compiler.eval(Compiler.java:6657)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7084)
     [java] 	at clojure.lang.Compiler.loadFile(Compiler.java:7040)
     [java] 	at clojure.main$load_script.invoke(main.clj:274)
     [java] 	at clojure.main$script_opt.invoke(main.clj:336)
     [java] 	at clojure.main$main.doInvoke(main.clj:420)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
     [java] 	at clojure.lang.Var.invoke(Var.java:379)
     [java] 	at clojure.lang.AFn.applyToHelper(AFn.java:154)
     [java] 	at clojure.lang.Var.applyTo(Var.java:700)
     [java] 	at clojure.main.main(main.java:37)
Comment by Ghadi Shayban [ 13/Feb/14 8:23 AM ]

Can you give some details about your JVM/environment that can help reproduce? I'm not encountering this error.

Comment by Tassilo Horn [ 13/Feb/14 9:41 AM ]

Sure. It's a 64bit ThinkPad running GNU/Linux.

% java -version
java version "1.7.0_51"
OpenJDK Runtime Environment (IcedTea 2.4.5) (ArchLinux build 7.u51_2.4.5-1-x86_64)
OpenJDK 64-Bit Server VM (build 24.51-b03, mixed mode)
Comment by Ghadi Shayban [ 13/Feb/14 10:19 AM ]

Strange, that is exactly my mail env, OpenJDK7 on Arch, 64-bit. I have also tested on JDK 6/7/8 on OSX mavericks. Are you certain that the git tree is clean besides the patch? (Arch users unite!)

Comment by Tassilo Horn [ 14/Feb/14 1:13 AM ]

Yes, the tree is clean. But now I see that I get the same error also after resetting to origin/master, so it's not caused by your patch at all. Oh, now the error vanished after doing a `mvn clean`! So problem solved.

Comment by Nicola Mometto [ 19/Feb/14 12:32 PM ]

Ghandi, FnExpr.parse should bind IN_TRY_BLOCK to false before analyzing the fn body, consider the case

(try (do something (fn a [] (heap-consuming-op a))) (catch Exception e ..))

Here in the a function the this local will never be cleared even though it's perfectly safe to.
Admittedly this is an edge case but we should cover this possibility too.

Comment by Ghadi Shayban [ 19/Feb/14 2:06 PM ]

You may have auto-corrected my name to Ghandi instead of Ghadi. I wish I were that wise =)

I will update the patch for FnExpr (that seems reasonable), but maybe after 1.6 winds down and the next batch of tickets get scrutiny. It would be nice to get input on a preferred approach from Rich or core after it gets vetted – or quite possibly not vetted.

Comment by Nicola Mometto [ 19/Feb/14 6:11 PM ]

hah, sorry for the typo on the name

Seems reasonable to me, in the meantime I just pushed to tools.analyzer/tools.emitter complete support for "this" clearing, I'll test this a bit in the next few days to make sure it doesn't cause unexpected problems.

Comment by Andy Fingerhut [ 24/Feb/14 12:13 PM ]

Patch CLJ-1250-AllInvokeSites-20140204.patch no longer applies cleanly to latest master as of Feb 23, 2014. It did on Feb 14, 2014. Most likely some of its context lines are changed by the commit to Clojure master on Feb 23, 2014 – I haven't checked in detail.

Comment by Ghadi Shayban [ 20/Mar/14 4:39 PM ]

Added a patch that 1) applies cleanly, 2) binds the IN_TRY_EXPR to false initially when analyzing FnExpr and 3) uses RT.booleanCast





[CLJ-1372] Inconsistent hash with java collections Created: 09/Mar/14  Updated: 20/Mar/14

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

Type: Defect Priority: Major
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: collections
Environment:

1.6.0 master


Attachments: File clj-1372-2.diff     File clj-1372.diff    
Patch: Code and Test

 Description   

c.c/hash always use hashCode for java collections, which is incompatible when comparing with Clojure collections, which use Murmur3.

user=> (== (hash (java.util.ArrayList. [1 2 3])) (hash [1 2 3]))
false
user=> (= (java.util.ArrayList. [1 2 3]) [1 2 3])
true

One way to fix it is to add a special case in Util/hasheq for java.util.Collections, as it is now for Strings.



 Comments   
Comment by Jozef Wagner [ 09/Mar/14 8:02 AM ]

Same problem for maps, so hasheq should have a special case for java.util.Map too.

Comment by Jozef Wagner [ 09/Mar/14 9:21 AM ]

Added patch with fix for j.u. Map, Set and List.

Comment by Andy Fingerhut [ 09/Mar/14 6:02 PM ]

Add patch clj-1372-2.diff that is identical to Jozef Wagner's clj-1372.diff, except it also adds some new tests that fail without his changes, and pass with them.

Comment by Alex Miller [ 10/Mar/14 9:31 AM ]

I think the contract on equiv/hasheq is more narrowly scoped than this and only applies if both collections are IPersistentCollection. In other words, I don't think this is wanted or required.

Note that the Java .equals/.hashCode contract is maintained here - these collections will compare as .equals() and do have the same .hashCode().

Comment by Jozef Wagner [ 10/Mar/14 9:38 AM ]

Without the patch the following statement is not valid: "If two objects are equal with c.c/=, than their hash returned by c.c/hash is the same number". We can say that this is valid only iff both objects are 'clojure' objects, but this goes against clojures interop principles (interop is easy, fast, no surprises).

Comment by Jozef Wagner [ 10/Mar/14 9:54 AM ]

Manifestation of this bug

user=> (assoc (hash-map [1 2 3] :foo) (java.util.ArrayList. [1 2 3]) :bar)
{[1 2 3] :bar, [1 2 3] :foo}
user=> (get (hash-map [1 2 3] :foo) (java.util.ArrayList. [1 2 3]))
nil
Comment by Alex Miller [ 10/Mar/14 10:58 AM ]

I agree that would be a nice thing to say without qualification.

There is a real cost to adding more branches in hasheq - adding those collection checks affects every hasheq. Running a full Clojure build, I see the following set of classes with >100 occurences where this happens (note that exactly 0 of these are the Java collections - this case doesn't exist in the Clojure build itself):

clojure.lang.Var 107001502
java.lang.Class 2651389
java.lang.Character 2076322 
java.util.UUID 435235 
java.util.Date 430956
clojure.lang.Compiler$LocalBinding 116830
java.lang.Boolean 112361
java.util.regex.Pattern 325

We'd be adding 4 more instanceof checks in the path of every one of those hasheqs. This would also likely blow any JVM inlining.

Rich says "all bets should be off for hasheq/equiv of non-values" where Java collections obviously fall into the class of "non-values".

Comment by Andy Fingerhut [ 10/Mar/14 11:04 AM ]

Would a doc patch be considered? Say one that modified the doc of clojure.core/hash to include a phrase indicating that it is only promised to be consistent with clojure.core/= for immutable values? It could even perhaps mention that Floats are out, too: see CLJ-1036

Comment by Alex Miller [ 10/Mar/14 12:00 PM ]

I think it would be preferred to do any detailed docs about hash at http://clojure.org/data_structures rather than in the docstring. Although the docstring on hash probably could use an update and a pointer to the web site after the latest changes.

Comment by Jozef Wagner [ 10/Mar/14 12:14 PM ]

Neverthless it is a breaking change from 1.5, and it should be mentioned in changelog. What still bugs me is that c.c/= is supported in such cases but the c.c/hash is not. If supporting c.c/hash is expensive, isn't it better to drop support for c.c/= in such cases? It will eliminate surprises such as:

user=> (apply distinct? (hash-set [1 2 3] (java.util.Collections/unmodifiableList [1 2 3])))
false
Comment by Alex Miller [ 10/Mar/14 2:05 PM ]

I'm not sure it's a "breaking" change if something not considered to be guaranteed changes. But I take your point.

I don't think it's feasible to drop = support for Clojure and Java collections - that seems important and useful. And if it were free to do so, I would like to be able to say without qualification that if equiv=true, then hasheq is the same.

It's unclear to me that the examples listed on this ticket are actually real problems people are likely to encounter. The main users of hasheq are hash map and hash set. So to manifest, you would need to be putting a mixture of Clojure and Java collections into one of those, in particular a mixture of collections that compare as equal.

Still thinking about it.

Comment by Jozef Wagner [ 10/Mar/14 3:27 PM ]

Sorry for spamming but there may be another option, to not fallback into hashCode in hasheq, but to instead throw in cases where hasheq is requested for non-values. This will lead to a cleaner separation of hash types. Of course it will prevent putting non-values into hash-set.

Comment by Alex Miller [ 10/Mar/14 3:34 PM ]

There is no simple check for "valueness" though?

Comment by Andy Fingerhut [ 10/Mar/14 3:37 PM ]

An idea, for what it might be worth: Add one test for instance of java.util.Collection in Util.hasheq method instead of 3 separate tests for Set, List, and Map. It doesn't cover Map.Entry.

Comment by Alex Miller [ 10/Mar/14 3:38 PM ]

Map doesn't extend Collection either.

Comment by Stuart Halloway [ 11/Mar/14 10:44 AM ]

I think this needs more consideration and should not hold up 1.6.

Comment by Andy Fingerhut [ 20/Mar/14 2:01 PM ]

Both patches clj-1372.diff and clj-1372-2.diff fail to apply cleanly as of latest Clojure master on Mar 20 2014. They did apply cleanly before the Mar 19 2014 commit, I believe, and the only issue appears to be a changed line of diff context. Given the discussion about whether such a change is desired, it sounds like more thought is needed before deciding what change should be made, if any.





[CLJ-1389] Re-loading a namespace ignores metadata specified for the namespace Created: 20/Mar/14  Updated: 20/Mar/14

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

Type: Defect Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: metadata, namespace, repl


 Description   

Using the REPL I added some metadata to a namespace and reloaded it.

(ns io.aviso.rook-test5)

to

(ns io.aviso.rook-test5
  "A testing namespace"
  {:inherted   :namespace
   :overridden :namespace})

But requesting the meta data yields nil:

(-> 'io.aviso.rook-test5 find-ns meta)
=> nil

I have tested a few variations, such as putting the metadata on the symbol instead of providing an attribute map. In all cases, the metadata from before the load persists.

Using remove-ns before re-loading the namespace does the right thing ... the metadata shows up as expected.






[CLJ-1388] equality bug on records created with nested calls to map->record Created: 18/Mar/14  Updated: 18/Mar/14

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

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

Attachments: Text File 0001-FIX-CLJ-1388.patch    
Patch: Code and Test
Approval: Triaged

 Description   
user> (defrecord a []) 
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2)  ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2)
#user.a{:a 1}

This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

Approach: Clean the extmap before putting it into the record constructor.

Patch: 0001-FIX-CLJ-1388.patch

Screened by:






[CLJ-1382] Vector sort order should be specified sufficiently to embrace sort-by-juxt Created: 13/Mar/14  Updated: 15/Mar/14  Resolved: 15/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: data-structures, documentation, idiom


 Description   

Vectors of equal length sort in a way that seems natural – by
comparing their 0th elements, then their 1st, etc., until something
is different:

user> (def vv '(["c" 9] ["a" 100] ["a" 33] ["b" 8]))
#'user/vv
user> (sort vv)
(["a" 33] ["a" 100] ["b" 8] ["c" 9])

This property enables a blisteringly wonderful idiom for sorting
records by multiple keys:

user> (def mm (->> vv (map (fn [[p q]] {:p p :q q}))))
#'user/mm
user> (sort-by (juxt :p :q) mm)
({:p "a", :q 33} {:p "a", :q 100} {:p "b", :q 8} {:p "c", :q 9})

The sort-by-juxt idiom was described on briancarper.net[1], where it
was refined for Clojure 1.1 by Malcolm Sparks. Andy Fingerhut has
also written about it, thoroughly.[2]

Such lore gives it the odor of respectability, but the sort-by-juxt
idiom takes liberties beyond the documented behavior ("contract") of
vectors. APersistentVector indulges the idiom, but the clojure.org
Data Structures page does not say how vectors should compare.

The vector specification should be bolstered with enough traits of
vectors' sorting behavior to make the sort-by-juxt idiom safe to use
wherever Clojure might be implemented.

[1] http://briancarper.net/blog/488/sort-a-clojure-map-by-two-or-more-keys

[2] https://github.com/jafingerhut/thalia/blob/master/doc/other-topics/comparators.md



 Comments   
Comment by Alex Miller [ 13/Mar/14 9:52 PM ]

I don't understand what this ticket is asking for.

Comment by Andy Fingerhut [ 13/Mar/14 10:32 PM ]

It sounds like he is asking that the doc of clojure.core/compare say that vectors of equal length are compared in lexicographic order.

Comment by Phill Wolf [ 15/Mar/14 1:07 PM ]

"(sort-by (juxt" relies on a feature of vectors that the Clojure documentation does not guarantee. But using juxt in this way is part of the cultural fabric and helps make concise programs that "read like a definition" of the problem, to quote Halloway in "Programming Clojure". Therefore, let's document the sort order of equal-length vectors. I looked for this information on the Data Structures page, which has a section on vectors.

Comment by Alex Miller [ 15/Mar/14 11:11 PM ]

I added a line to the Vectors section on the Data Structures (http://clojure.org/data_structures) page: "Vectors are compared first by length, then each element is compared in order."





[CLJ-1383] Should name throw on nil? Created: 14/Mar/14  Updated: 15/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: John Chijioke Assignee: Jozef Wagner
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File clj-1383.diff    
Patch: Code and Test

 Description   

The name function throws NullPointerException on nil. Since the name function is about obtaining the string form of a specific object it should not throw on nil. It should just return the nil object as the str fn does.



 Comments   
Comment by Jozef Wagner [ 15/Mar/14 3:23 AM ]

added patch with test





[CLJ-1381] Improve support for extending protocols to primitive arrays Created: 13/Mar/14  Updated: 13/Mar/14

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

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


 Description   

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

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

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

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

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

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






[CLJ-1378] Hints don't work with #() form of function Created: 11/Mar/14  Updated: 12/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Roy Varghese Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: interop, typehints

Attachments: File clj-1378.diff     File clj-1378-v2.diff    
Patch: Code
Approval: Triaged

 Description   
;; WORKS
(deftest test-add-job
  (let [pool (java.util.concurrent.Executors/newFixedThreadPool 10)
        counter 50000
        f (fn [])]
    (dotimes [i counter]
      (.submit pool ^Runnable  f ))
    (Thread/sleep 1000)
    (is (= counter (count (all-jobs))))))

;; FAILS
(deftest test-add-job
  (let [pool (java.util.concurrent.Executors/newFixedThreadPool 10)
        counter 50000]
    (dotimes [i counter]
      (.submit pool ^Runnable  #()))
    (Thread/sleep 1000)
    (is (= counter (count (all-jobs))))))

Caused by: java.lang.IllegalArgumentException: More than one matching method found: submit
	at clojure.lang.Compiler.getMatchingParams(Compiler.java:2380)
	at clojure.lang.Compiler$InstanceMethodExpr.<init>(Compiler.java:1412)
	at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:952)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6560)
	... 87 more


 Comments   
Comment by Jozef Wagner [ 12/Mar/14 4:03 AM ]

Functions do have metadata, but Compiler does not look in them for type hints.

user=> (with-meta #() {:foo :bar})
#<clojure.lang.AFunction$1@779325ee>

When compiler is determining which native method to use, it matches method signature with classes of given args. There is a getJavaClass() method in Compiler.java which returns a class for given expression. Vars expressions and local bindings use :tag metadata to override this class, but most other expressions don't. Compiler parses #() into a FnExpr, which always return AFunction as its class.

Most of time this approach is OK, as AFunction implements Runnable and Callable so there is no need for type hint. However, in this particular case, there are overrides for both Runnable and Callable, and as AFunction can be either of them, the expression is ambiguous.

Comment by Jozef Wagner [ 12/Mar/14 4:17 AM ]

Patch added, following expression will now run without error

(.submit (java.util.concurrent.Executors/newCachedThreadPool) ^Runnable #())
Comment by Alex Miller [ 12/Mar/14 9:34 AM ]

Could you add a test to the patch?

Comment by Jozef Wagner [ 12/Mar/14 2:53 PM ]

Attached patch clj-1378-v2.diff which contains both fix and test.





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

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

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

All clojure versions


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

 Description   

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

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

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






[CLJ-1377] java.lang.IllegalArgumentException: More than one matching method found on calling ExecutorService.submit from let instead of using Var. Created: 11/Mar/14  Updated: 12/Mar/14  Resolved: 11/Mar/14

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

Type: Defect Priority: Major
Reporter: Roy Varghese Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Example from Joy Of Clojure doesn't work when Executor service pool is used in (let )

https://groups.google.com/forum/#!msg/clojure/asEXM2uHyxw/aK4rrKOKs1YJ



 Comments   
Comment by Alex Miller [ 11/Mar/14 3:47 PM ]

In the let case, the pool will be tagged with the proper type so the ambiguity is detected.

In the def case, the pool will be seen as an object and the compiler is just deferring to reflection at runtime to figure it out. If you turn on warn-on-reflection, you'll see a reflection warning in this case. Reflection is just picking the first one that matches in that case. If you type hinted the def case, you'd see the same error.

I don't think there is a bug here.

Comment by Roy Varghese [ 11/Mar/14 5:08 PM ]

Actually, I did turn on warn-on-reflection, but didn't see any messages.

I think this creates an element of surprise and uncertainty.

(def A (let [x (java.util.concurrent.Executors/newFixedThreadPool 5)] x))

How would (.submit A ...) behave?

Comment by Alex Miller [ 11/Mar/14 11:15 PM ]

When I ran the prior example, I saw a reflection warning.

In your new example, I would expect x to be typed as a ThreadExecutorPool, A to be typed as an Object, and .submit to get called reflectively (successfully) and produce a reflection warning. It's effectively no different than the def example in the post.

What are you suggesting is the bug and what should happen? Reflector could throw an error when it finds multiple matches but that would certainly break code that is currently working like the example you gave, so I doubt we would consider such a change at this point. There really is an ambiguity here, so I think a type hint is required to make it work unambiguously in either case.

Comment by Roy Varghese [ 12/Mar/14 10:49 AM ]

The bug is that type hints are required in one case, and not required in the other case because of implementation details, unless its documented in the language that (def) and (let) hold different types of Vars.

I the example above, I would expect A and x to refer to the same object, and thus contain the type information. Or not. Without reading the implementation, either case seems possible.

Agree, its a breaking change, but that's a different consideration.

Comment by Roy Varghese [ 12/Mar/14 10:50 AM ]

BTW..trying to fix with hints is what led to CLJ-1378.

Comment by Alex Miller [ 12/Mar/14 11:39 AM ]

x is locally bound to an object (there is no Var here) - the type information is on the local binding, inferred from the type of the expression. In the compiled form, the let expression does know the return type of the evaluated let (if there were multiple branches, there might be many possible return types). The def creates a Var that holds the result of evaluating the let. At compilation time, the type of the result of the let is unknown so is assumed to be Object.

So, x is a local binding and A is a Var. Even if they both refer to the same object, they do so in different contexts using different information. The type hinting and binding behavior in let is described on the special forms page http://clojure.org/special_forms. Remember too that Clojure is a dynamic language - while the Var A might hold a ThreadPoolExecutor instance now, it might hold something else later.

CLJ-1378 is useful - that's phrased more as a problem we can solve and there's a reasonable patch there.

Comment by Alex Miller [ 12/Mar/14 11:55 AM ]

"there is no Var here" == "there is no Var at this point"
"the let expression does know the return type" should have been "does NOT know"





[CLJ-1365] New collection hash functions are too slow Created: 20/Feb/14  Updated: 11/Mar/14  Resolved: 11/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: collections

Attachments: Text File clj-1365-v1.patch     Text File clj-1365-v2.patch     Zip Archive testclj1365.zip    
Patch: Code
Approval: Ok

 Description   

As reported ( https://groups.google.com/d/msg/clojure-dev/t6LAmVe-RLM/ekLTKxYfU5UJ ) by Mark Engelberg, the new collection hashing functions are slower than invoking the Murmur3 functions directly. See the attached zip for performance tests.

Approach: Made mix-collection-hash, hash-ordered-coll, and hash-unordered-coll use primitive type hints to avoid the bulk of the time.

Patch: clj-1365-v2.patch

Screened by:



 Comments   
Comment by Alex Miller [ 20/Feb/14 11:26 AM ]

Added to 1.6 release.

Comment by Alex Miller [ 20/Feb/14 12:40 PM ]

Made hash functions inline for performance.

Comment by Rich Hickey [ 20/Feb/14 7:55 PM ]

Reported where?

This looks like bad benchmarking.

(dotimes [_ 10] (let [x 1 y 1] (time (dotimes [n 1000000000] (clojure.lang.Murmur3/mixCollHash x y)))))

and

(dotimes [_ 10] (let [x 1 y 1] (time (dotimes [n 1000000000] #_(clojure.lang.Murmur3/mixCollHash x y)))))

take the same time on my machine.

I'd need to see tests where the return was definitely used, it seems this is just more easily ignored by hotspot when not used.

We probably only need to hint count and the return for decent results.

Comment by Alex Miller [ 20/Feb/14 8:55 PM ]

It was reported by Mark Engelberg in his Instaparse rework - he observed these calls taking noticeably longer and overall times 10-20% down. I will ask him to chime in here.

Comment by Rich Hickey [ 04/Mar/14 8:44 AM ]

Could someone please test hinting hint count and the return? I'd hate for the answer to anyone's perf issues be inlining.

Comment by Alex Miller [ 04/Mar/14 9:06 AM ]

I will provide some more data for consideration of the options.

Comment by Alex Miller [ 04/Mar/14 11:07 AM ]

Test project for different variants

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

Attached a test project with different variants for testing and better benchmarking. To run:

unzip testclj1365.zip
cd clj1365
lein uberjar
java -server -cp target/clj1365-0.1.0-SNAPSHOT-standalone.jar clj1365.core

Results:

mix-collection-hash original
"Elapsed time: 57.777 msecs"
"Elapsed time: 18.034 msecs"
"Elapsed time: 20.591 msecs"
"Elapsed time: 25.179 msecs"
"Elapsed time: 21.781 msecs"
mix-collection-hash hints
"Elapsed time: 14.983 msecs"
"Elapsed time: 8.871 msecs"
"Elapsed time: 8.793 msecs"
"Elapsed time: 8.92 msecs"
"Elapsed time: 8.873 msecs"
mix-collection-hash inline
"Elapsed time: 10.04 msecs"
"Elapsed time: 7.117 msecs"
"Elapsed time: 7.306 msecs"
"Elapsed time: 7.324 msecs"
"Elapsed time: 7.175 msecs"
Murmur3/mixCollHash
"Elapsed time: 9.522 msecs"
"Elapsed time: 7.288 msecs"
"Elapsed time: 7.397 msecs"
"Elapsed time: 7.364 msecs"
"Elapsed time: 7.345 msecs"

From these results, I infer that the unhinted version is slower (21 ms) than a static call (7 ms). Inlining gives you same perf as static. Hinting inputs and return gives almost the same perf (9 ms).





[CLJ-1376] Initialize internal maps to more efficient version Created: 11/Mar/14  Updated: 11/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: performance


 Description   

In reviewing some hashing stuff, I noticed that there are many places internal to Clojure that use maps initialized with PersistentHashMap.EMPTY. Many of these maps are likely to have a small number of entries such that a PersistentArrayMap might be more efficient.

These are the candidates:

src/jvm/clojure/lang/ARef.java
19:private volatile IPersistentMap watches = PersistentHashMap.EMPTY;

src/jvm/clojure/lang/Compiler.java
3009:				IPersistentMap m = PersistentHashMap.EMPTY;
3819:					       KEYWORDS, PersistentHashMap.EMPTY,
3820:					       VARS, PersistentHashMap.EMPTY,
3964:	IPersistentMap closes = PersistentHashMap.EMPTY;
3977:	IPersistentMap keywords = PersistentHashMap.EMPTY;
3978:	IPersistentMap vars = PersistentHashMap.EMPTY;
5121:                            ,CLEAR_SITES, PersistentHashMap.EMPTY
7259:			       KEYWORDS, PersistentHashMap.EMPTY,
7260:			       VARS, PersistentHashMap.EMPTY
7418:			IPersistentMap opts = PersistentHashMap.EMPTY;
7475:			IPersistentMap fmap = PersistentHashMap.EMPTY;
7522:					       KEYWORDS, PersistentHashMap.EMPTY,
7523:					       VARS, PersistentHashMap.EMPTY,
7912:                            ,CLEAR_SITES, PersistentHashMap.EMPTY

src/jvm/clojure/lang/LispReader.java
755:					RT.map(GENSYM_ENV, PersistentHashMap.EMPTY));

src/jvm/clojure/lang/MultiFn.java
39:	this.methodTable = PersistentHashMap.EMPTY;
41:	this.preferTable = PersistentHashMap.EMPTY;
49:		methodTable = methodCache = preferTable = PersistentHashMap.EMPTY;

src/jvm/clojure/lang/Var.java
48:	final static Frame TOP = new Frame(PersistentHashMap.EMPTY, null);
175:	setMeta(PersistentHashMap.EMPTY);
341:	IPersistentMap ret = PersistentHashMap.EMPTY;

Approach: Two possible approaches - initialize to PersistentArrayMap.EMPTY or call RT.map(). The latter requires function invocation so is slightly slower, but has the benefit of localizing map construction into a single place.






[CLJ-1375] Remove Util.pcequiv() and stop pretending Java colls are equiv to Clojure colls Created: 11/Mar/14  Updated: 11/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File clj-1375-v1.patch    
Patch: Code

 Description   

The Util.pcequiv() method

static public boolean pcequiv(Object k1, Object k2){
	if(k1 instanceof IPersistentCollection)
		return ((IPersistentCollection)k1).equiv(k2);
	return ((IPersistentCollection)k2).equiv(k1);
}

tries to get equiv semantics (cross-class number equality) for cases of mixed Clojure/Java collection comparison. However, this is not a sustainable direction and we would like to stop doing this.

Attached patch removes this and changes calling code to only call equiv when both collections are IPersistentCollection.






[CLJ-1371] divide(Object, Object) with (NaN, 0) does not return NaN Created: 07/Mar/14  Updated: 07/Mar/14

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

Type: Defect Priority: Trivial
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math


 Description   

user=> (def x Double/NaN)
#'user/x
user=> (/ x 0)

ArithmeticException Divide by zero clojure.lang.Numbers.divide (Numbers.java:156)
user=> (/ Double/NaN 0)
Double/NaN



 Comments   
Comment by Alex Miller [ 07/Mar/14 7:50 AM ]

As per the Java Language Specification (http://docs.oracle.com/javase/specs/jls/se7/html/jls-4.html#jls-4.2.4),

"All numeric operations with NaN as an operand produce NaN as a result."

Comment by Yongqian Li [ 07/Mar/14 7:54 AM ]

But in the first example it produces an ArithmeticException.

Comment by Alex Miller [ 07/Mar/14 9:27 AM ]

Ah, I see the question now.

Here we are dividing a double by a long. In the first case, this is parsed as divide(Object, long) which then calls divide(Object, Object), which throws ArithmeticException if the second arg is 0 (regardless of the first arg).

In the second case it's parsed as divide(double, long) which just relies on Java to properly upcast the primitive long to a double to do the divide.

Note that making this call with 2 doubles does return NaN:

user=> (def x Double/NaN)
#'user/x
user=> (/ x 0.0)
NaN

or type hinting x to a double works as well:

user=> (def x Double/NaN)
#'user/x
user=> (/ ^double x 0.0)
NaN

I think one option to "fix" this behavior would be to add checks in divide(Object, Object) to check whether x is NaN and instead return NaN.





[CLJ-1370] ((println)) should throw CompilerException instead of NPE? Created: 06/Mar/14  Updated: 06/Mar/14  Resolved: 06/Mar/14

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

Type: Defect Priority: Minor
Reporter: The Alchemist Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug


 Description   

How to Reproduce

=> ((println))

NullPointerException   hello.core/eval4117 (NO_SOURCE_FILE:1)

What's Wrong?

I might be completely wrong, in which case don't hesitate to close this defect, but I wish this would throw a CompilerException with an IllegalArgumentException, just like ((nil)):

=> ((nil))
CompilerException java.lang.IllegalArgumentException: Can't call nil, compiling:(NO_SOURCE_PATH:1:2)


 Comments   
Comment by Alex Miller [ 06/Mar/14 12:37 PM ]

There is no compilation error here. The error occurs during evaluation.

user> (defn x [] ((println)))  ;; compiles just fine
#'user/x
user> (x)   ;; fails in evaluation
NullPointerException   user/x (NO_SOURCE_FILE:1)

The error is thrown when trying to evaluate (nil) where NullPointerException is a perfectly valid error.





[CLJ-944] Compiler sometimes gives constant collections types which mismatch with their runtime values Created: 04/Mar/12  Updated: 06/Mar/14  Resolved: 06/Mar/14

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

Type: Defect Priority: Major
Reporter: Alf Kristian Støyle Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: compiler
Environment:

Clojure 1.3 om Mac OS 10.7, Clojure 1.5.0 alpha1 on Linux x86_64 (OpenJDK 1.7.0 b147)


Attachments: Text File 0001-Fix-for-CLJ-944.patch     Text File 0002-Fix-for-CLJ-944.patch     Text File clj944-plus-tests.patch    
Patch: Code and Test
Approval: Ok

 Description   
(.containsKey {:one 1} :one)
;=> ClassCastException clojure.lang.PersistentArrayMap cannot be cast to clojure.lang.PersistentHashMap

The map is a clojure.lang.PersistentArrayMap, which obviously has a containsKey method (https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/PersistentArrayMap.java#L95).

Casting it works fine though:

(.containsKey ^clojure.lang.PersistentArrayMap {:one 1} :one)
;=> true

The problem is not present in Clojure 1.2.1.

Cause: In the first example, the Compiler will parse the MapExpr (which reports a Java class of IPersistentMap) into a ConstantExpr whose value is a PersistentHashMap (and reports type as same). The .containsKey() call is a HostExpr which parses to an InstanceMethodExpr.

When emitted to bytecode, the ConstantExpr becomes a call to RT.map() to generate a map. RT.map() produces either a PersistentArrayMap or PersistentHashMap depending on size. The InstanceMethodExpr becomes a call to PersistentHashMap.containsKey() (based on the reported type of it's expression). In the case where RT.map() will produce a PersistentArrayMap at runtime (small maps), this causes a mismatch.

Note also that this same scenario occurs for constant sets, vectors, etc - all of those cases produce ConstantExpr's with the type of the concrete data structure, instead of the more general IPersistentMap, IPersistentSet, etc. All of those scenarios should be addressed in the same way. But, those interfaces do not capture the full capabilities of the returned object. The compiler and the marshaller need to agree on the types to produce.

Approach: One approach (A) explored previously was to have MapExpr always produce a ConstantExpr containing a value of the same type that will be produced at runtime by RT.map(). While this works, it is also fragile, and sidesteps the real issue that MapExpr creates a ConstantExpr with a type that is too specific. Rich said this is the wrong approach.

(B) Another approach is to create an anonymous subclass of ConstantExpr in MapExpr that overrides getJavaClass() to report a more general type. This isn't done anywhere else and it may have negative ramifications (but I have not found any).

(C) Another approach is to follow the path of example #2 above. That example wraps the map in a MetaExpr which reports getJavaClass() returning the hinted type. In the case above, it hints the concrete type that happens to match the return of RT.map(). Instead, we could wrap the ConstantExpr in a MetaExpr hinted on the abstract type IPersistentMap (the return type of RT.map()).

RH - a more general (interface) type will not give full capabilities. Perhaps APersistent*

Patch:



 Comments   
Comment by Nicola Mometto [ 30/Oct/12 5:02 PM ]

The attached patch fixes the issue, by emitting IPersistentMap instead of Persistent{Hash|Array}Map as class type for maps literals

Comment by Nicola Mometto [ 01/Nov/12 3:48 PM ]

I uploaded another patch fixing the same problem in a different way.
While 0001-Fix-for-CLJ-944.patch makes clojure.lang.Complier.ConstantExpr#getJavaClass return clojure.lang.IPersistentMap for both clojure.lang.PersistentHashMap and clojure.lang.PersistentArrayMap, 0002-Fix-for-CLJ-944.patch makes clojure.lang.Compiler.MapExpr#parse return a PersistentArrayMap if the length is <= HASHTABLE_THRESHOLD, instead of always returning a PersistentHashMap.

This approach is more consistent, making the type of the compiler's internal representation of a map literal equal to the one of the reader.

Note that this second approach while being more consistent, breaks some tests that assume some operations on maps (specifically `seq` and `print`) to be order dependent, and written with the hash-map return order implementation in mind.

That should not be the case and if the second patch is preferred over the first one, I'll gladly fix those tests.

Comment by Stuart Halloway [ 01/Mar/13 12:09 PM ]

Approach #2, relying on consistent choice of concrete map class by size throughout, feels quite fragile.

Approach #1 seems to abuse the method name getJavaClass(), now having it return "get the base type I would need for cast".

Maybe there needs to be a different thing entirely?

Comment by Nicola Mometto [ 01/Mar/13 2:17 PM ]

Patch #2 should get merged (IMHO) regardless of the fragility of its approach to fixing this ticket's bug, since it fixes another bug:

prior to the patch:

user=> (class {:a 1})
clojure.lang.PersistentArrayMap
user=> (def a {:a 1})
#'user/a
user=> (class a)
clojure.lang.PersistentHashMap

after the patch:

user=> (class {:a 1})
clojure.lang.PersistentArrayMap
user=> (def a {:a 1})
#'user/a
user=> (class a)
clojure.lang.PersistentArrayMap

This should also lead to some minor performance enhancement since prior to this moment, every map def'ed would be a HashMap instead of an ArrayMap

So, I think patch #2 should be applied if not for this ticket's bug, at least for the reason stated above.
If somebody has any proposal for making this patch more solid regarding this ticket's bug, any help is welcome

Comment by Rich Hickey [ 13/Apr/13 9:41 AM ]

This should not have passed screening. There are two issues, should be separate. I have no idea what has been screened nor what will be applied should it be approved. There's contention in the discussion but no resolution.

Comment by Nicola Mometto [ 13/Apr/13 12:06 PM ]

I don't think that there are two issues here.
The issue is only one: the compiler doesn't emit maps in a way consistent with what the reader returns and with how the compiler itself uses maps.
The symptoms are two: some interop calls fail, and def'ed vars with a literal map as value never use a PersistentArrayMap.

The underlying cause of those two symtoms is fixed by the patch 002 that i submitted (incorporated in Stu's clj944-plus-tests patch.

Stuart said that this approach feels fragile but the bug is caused by the fact that everywhere else clojure returns a PersistentArrayMap when the element count is <= than the PersistentHashMap threshold, and when emitting maps, it doesn't.

Making clojure emit maps consistently with how clojure does internally everywhere else looks to me like the only solution, and I don't really see how making clojure consistent is a fragile approach.

But again, if somebody can suggest a better solution to this problem, I'll gladly submit another patch.

Comment by Rich Hickey [ 14/Aug/13 8:36 AM ]

"Compiler makes different concrete maps then the reader" is not inherently a problem. But the code posted is a problem:

(.containsKey {:one 1} :one)
;=> ClassCastException clojure.lang.PersistentArrayMap cannot be cast to clojure.lang.PersistentHashMap

i.e. .containsKey isn't working in this scenario. Stating exactly why is the path to a good discussion and fix.

Comment by Nicola Mometto [ 14/Aug/13 10:01 AM ]

I don't know how you're getting that exception.

This patch removes exactly that problem, I just tried it out on a fresh clojure git repo with only that patch applied and it works:
Clojure 1.6.0-master-SNAPSHOT
user=> (.containsKey {:one 1} :one)
true

Comment by Alex Miller [ 16/Aug/13 10:49 AM ]

Nicola, I believe Rich is saying that:

1) the posted original example is showing a real problem
2) the title of the ticket and the suggested cause (compiler makes different concrete maps than the reader) is incorrect and that it is ok for them to make different concrete maps
3) the original error message (.containsKey isn't working) is the path to a correct diagnosis.

Rich is NOT saying that the patch doesn't resolve the error but rather that it resolves it in an undesired way due to a misdiagnosis of cause.

Comment by Nicola Mometto [ 17/Aug/13 7:38 AM ]

I'm going to try to describe as clearly as possible what's the problem and what could be the possible solutions (and thus what made me chose this as the best possible fix)

Problem: .containsKeys does not work on some maps

Diagnosis:
Constant maps are expressed as a ConstantExprs by the compiler; the ConstantExpr#getJavaClass method is implemented as a call to .getClass() on the constant object.

Since MapExpr#parse in case of a constant map always delegated to ConstantExpr a PersistentHashMap, it's clear that .getJavaClass on a ConstantExpr-wrapped map will always return PersistentHashMap.

This means that interop-calls will try to resolve PersistentHashMap methods instead of IPersistentMap (since they call .getJavaClass on the object to determine what to target).

When and why does this bug not manifest?

  • if the map is not constant:
    user=> (.containsKey {:one (Object.)} :one)
    true
  • if the map gets compiled as opposed to being .eval'ed
    usser=> (def a (.containsKey {:one 1} :one))
    #'user/a
    user=> a
    true

This also mean that currently, maps that are hold in a Var or that are AOT compiled are always PHMs and never PAMs (removing a possible optimization)

What are the possible solutions?

  • Don't wrap constant maps in ConstantExprs so we always use MapExpr and remove the problem.
  • Special-case ConstantExpr#getJavaClass to return IPersistentMap for PHMs and PAMs (approach #1)
  • Be consistent in MapExpr#parse and create PAMs when the element number is above the PHM threshold as we do EVERYWHERE but in there. (approach #2)

Of the three possible solutions, the third seems to me the best one.

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

Thanks for this - it was a very useful summary for me as I'm still trying to get my head around all the details of this ticket. Stepping back a bit, I'd say in your diagnosis that the point at which I see something wrong is when MapExpr#parse returns something that depends on a particular implementation type (via getJavaClass).

Idea: what if MapExpr#Expr instead returned a anonymous subclass of ConstantExpr that overrode getJavaClass() to return IPersistentMap in this case?

return new ConstantExpr(m) { 
  public Class getJavaClass() {
    return IPersistentMap.class;
} };

That way, the ConstantExpr only commits to the abstraction. This seems to also fix the problem to me.

Separately, I agree that it may be useful as an optimization to emit a PAM instead of a PHM in this case (maybe even in all constant cases?). But I think we should not rely on two different pieces of code producing the identical map impl - that should be hidden behind the abstraction (IPersistentMap). I think I would like to break that out as an independent ticket though.

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

I see that my suggestion is effectively similar to the first patch on this ticket but (imho) cleaner in that it puts the change at the point where the knowledge exists and can thus be a simpler check.

Comment by Andy Fingerhut [ 08/Nov/13 10:19 AM ]

Patch clj944-plus-tests.patch no longer applies cleanly to latest master, probably due to changed context lines in a test file, but I haven't checked. Not worth updating until/unless there is agreement on what should be changed.

Comment by Nicola Mometto [ 08/Nov/13 1:27 PM ]

I would very like to see this fixed and will happily update my patches, however I'd first like some feedback on the correct approach here before making yet another patch.

Should we go with one of my two approaches or should I implement what Alex suggested?
Each approach fixes this bug, I stated before what's causing this and I have explained my solutions, Alex's one looks reasonable too, can we please get an opinion on what's the best move to get a satisfying fix for this?

Thanks

Comment by Alex Miller [ 08/Nov/13 1:45 PM ]

Sorry it's not visible on the ticket, but I have actually spent several hours looking at this since my last comment but have not reached a final conclusion. I also would really like to see it fixed and have not given up on it yet.

Comment by Andy Fingerhut [ 08/Nov/13 1:53 PM ]

The most direct cause of the exception in the example of how to reproduce it is due to the compiler emitting a type check on a type that is different than the constant emitted.

This is just tossing out ideas, perhaps brain-dead ones, but eliminating that type check would eliminate the exception. Also, changing the emitted type check to be for a more general type than than the constant, e.g. IPersistentMap, should also eliminate the exception.

Comment by Nicola Mometto [ 08/Nov/13 2:27 PM ]

Alex, there's no need to be apologetic, I was in no way trying to imply a lack of interest, just asking for directions

Wrapping up for screeners/Rich:

Here is a diagnosis of the bug: http://dev.clojure.org/jira/browse/CLJ-944?focusedCommentId=31694&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-31694

To me now it looks like the best solutions right now are either:

  • My #2 solution: making MapExpr#parse return a PAM or a PHM consistently with RT/map
  • Alex's solution: making MapExpr#Expr return an anonymous subclass of ConstantExpr overriding getJavaClass() to return IPersistentMap

Right now my patch does not apply to master, and there is no patch implementing Alex's solution, once I get a feedback on which approach is preferable i can: update my patch to apply to master or implement Alex's solution (Alex, if you want to do it yourself I will not step in)

Comment by Alex Miller [ 08/Nov/13 2:33 PM ]

I have actually built a couple of variant patches locally but still have not convinced myself that any of them are the right patch to recommend. I hope to look at it again in the next couple weeks. I think the ticket itself is in the correct state (Incomplete). Ideally I would have a few minutes to look at it with Rich for further direction.

Comment by Nicola Mometto [ 20/Feb/14 6:50 PM ]

Any update on this? this is still marked for clojure 1.6

Comment by Alex Miller [ 20/Feb/14 8:59 PM ]

Actually yes! I spent some time on this today and have something to attach.

Comment by Nicola Mometto [ 22/Feb/14 12:28 PM ]

A problem I see with approaches b and c is that they don't address the issue I raised about (def a {:a 1}) here: http://dev.clojure.org/jira/browse/CLJ-944?focusedCommentId=29885&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-29885

Rich stated that it's a different issue than the one being discussed in this ticked, I don't agree, to me it's simply another symptom of the same bug.

I want to suggest a fourth alternative approach to this that combines approach -a with whichever is preferred between -b and -c.

Comment by Alex Miller [ 22/Feb/14 1:53 PM ]

Nicola, I'm not getting your point here (the linked comment doesn't seem to match up to the comments about (def a {:a 1}) so that's confusing me). In the case of (def a {:a 1}), my reading of the bytecode is that RT.map() (which returns IPersistentMap) will be invoked at runtime to generate the map. There is more than one potential concrete class for a (depending on size), but nothing is expecting it to be a particular concrete type here. I do not expect literal maps to retain order or be ArrayMaps (you should call array-map if you want that).

I'm hoping to get some consensus from Rich about the newly updated problem, cause, and approach before I move further. I'd currently classify the problem as the failed .containsKey call, the cause as the compiler replacing the MapExpr with an expression that specifies its type more narrowly than it should, and B and C as some candidate solutions.

If we want the compiler to generate concrete maps that match RT.map(), I think that's ok too, but I don't see it as the cause right now.

Comment by Nicola Mometto [ 22/Feb/14 2:25 PM ]

Alex, I'm sorry here's the correct link: http://dev.clojure.org/jira/browse/CLJ-944?focusedCommentId=30684&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-30684

Your analysis of the emitted bytecode is correct, however when the code is loaded JIT, calls to `def` will be interpreted https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L409-L434 as a result, if the initexpr is a ConstantExpr, the in-memory value of that ConstantExpr will be used and if it's the case that the expression is a map, that will always be a PHM.

I understand that switching between PAMs and PHMs is merely an optimization but having code behave differently if it's AOT compiled rather than JIT loaded is undesiderable IMHO.

Comment by Nicola Mometto [ 22/Feb/14 2:41 PM ]

Immediatelly after posting the previous response it occurred to me that approaches b and c have an inherent problem:
with that approach it's no longer possible to access public fields/methods in instances of PAMs or PHMs that do not belong in IPersistentMap without incurring in reflection.

This means that for example calls to java.lang.Map instance methods would require type-hinting the map literal while now it can be resolved at compile-time without the need of type-hinting.

Here's an example using a first patch that implements the fix as described by approaches -b or -c

user=>  (set! *warn-on-reflection* true)
true

;; pre patch
user=>  (.isEmpty {:a 1})
false

;; after patch
user=>  (.isEmpty {:a 1})
Reflection warning, NO_SOURCE_PATH:2:2 - reference to field isEmpty on clojure.lang.IPersistentMap can't be resolved.
false
Comment by Alex Miller [ 05/Mar/14 11:44 AM ]

Remove from 1.6 release. Still needs more analysis.

Comment by Alex Miller [ 06/Mar/14 7:53 AM ]

Reopened just to fix the ticket fields.





[CLJ-1369] CLJ-738 is marked Closed is not implemented Created: 04/Mar/14  Updated: 04/Mar/14  Resolved: 04/Mar/14

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

Type: Defect Priority: Minor
Reporter: David Welte Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

OS X, Java 6



 Description   

CLJ-738 is marked Closed but the attached patch is has not been applied and both Clojure 1.5.1 and 1.6.0-beta2 exhibit the bad behavior listed in CLJ-738. The issue that CLJ-738 is that (<= (Double. Double/NaN) 1) evaluates to true while (<= Double/NaN 1) evaluates to false. See CLJ-738 for many details.



 Comments   
Comment by Alex Miller [ 04/Mar/14 3:20 PM ]

Thanks for letting us know about this - I concur that 738 was incorrectly closed without being applied and I have resurrected that ticket. I am closing this one. In the future, feel free to just comment on a ticket directly, or better (for a closed ticket), comment on one of the mailing lists.





[CLJ-304] clojure.repl/source does not work with deftype Created: 20/Apr/10  Updated: 03/Mar/14

Status: In Progress
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: repl


 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.





[CLJ-1366] The empty map literal is read as a different map each time Created: 01/Mar/14  Updated: 02/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: OHTA Shogo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: memory, reader

Attachments: Text File 0001-make-the-reader-return-the-same-empty-map-when-it-re.patch     Text File 0002-make-the-reader-return-the-same-empty-map-when-it-re.patch    
Patch: Code

 Description   

As reported here (https://groups.google.com/forum/?hl=en#!topic/clojure-dev/n83hlRFsfHg), the empty map literal is read as a different map each time.

user=> (identical? (read-string "{}") (read-string "{}"))
false

Making the reader return the same empty map when it reads an empty map is expected to improve some memory efficiency, and also lead to consistency with the way other collection literals are read in.

user=> (identical? (read-string "()") (read-string "()"))
true
user=> (identical? (read-string "[]") (read-string "[]"))
true
user=> (identical? (read-string "#{}") (read-string "#{}"))
true

Cause: LispReader calls RT.map() with an empty array when it reads an empty map, and RT.map() in turn makes a new map unless its argument given is null.

Approach: make RT.map() return the same empty map when the argument is an empty array as well, not only when null



 Comments   
Comment by OHTA Shogo [ 01/Mar/14 2:59 AM ]

Sorry, the patch 0001-make-the-reader-return-the-same-empty-map-when-it-re.patch didn't work.

The updated patch 0002-make-the-reader-return-the-same-empty-map-when-it-re.patch works, but I'm afraid it'd be beyond the scope of this ticket since it modifies RT.map() behavior a bit.





[CLJ-1329] Unused local variable in PersistentVector.cons() Created: 22/Jan/14  Updated: 02/Mar/14

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

Type: Enhancement Priority: Trivial
Reporter: Smit Shah Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File fix.patch    
Patch: Code
Approval: Triaged

 Description   

in src/jvm/clojure/lang/PersistentVector.java:168, there is an integer i being defined which is not being used anywhere in the method.

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/PersistentVector.java#L168



 Comments   
Comment by Stuart Halloway [ 31/Jan/14 6:14 PM ]

Smit, can you please submit a CA? http://clojure.org/contributing

Comment by Smit Shah [ 02/Feb/14 1:16 PM ]

Stuart, I will send the CA via post ASAP.
It might take a couple of days to reach Rich though.

Comment by Smit Shah [ 01/Mar/14 11:51 AM ]

Stuart, I have successfully submitted the CA (http://clojure.org/contributing).
I guess now merging this patch shouldn't be a problem

Comment by Alex Miller [ 02/Mar/14 11:37 AM ]

Thanks Smit!





[CLJ-1368] Document usage for case with non-readable constants Created: 02/Mar/14  Updated: 02/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docs, interop


 Description   

Problem

It is pretty obscure how to get constant-time dispatch for e.g. Enums, even if user knows about case.

Proposal

The possibility to dispatch to arbitrary constants with case, by wrapper macro, should be documented.

Wording

  • Should it warn against doing that with unstable values?
  • Should it mention anything else than java Enums?

Case Techniques

Case is documented for accepting all readable forms as test-constants. However, it can also be made to use any compile-time-known constants as test-constants, by wrapping it in another macro.

Sometimes this is appropriate, e.g. when dispatching on a java Enum.
Other times, less so, e.g. when dispatching on objects whose hash changes when the vm is restarted (breaks AOT).

Implications

This technique is an application of a more general technique: Passing non-literals to a macro from another macro.
Are there other macros that have use cases like this?

References

https://groups.google.com/d/topic/clojure/3yGjDO2YnjQ/discussion



 Comments   
Comment by Herwig Hochleitner [ 02/Mar/14 11:25 AM ]

This is a duplicate of http://dev.clojure.org/jira/browse/CLJ-1367

Actually, it's an alternate solution





[CLJ-1367] Allow case statement to compare java constants Created: 02/Mar/14  Updated: 02/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: interop


 Description   

As raised on the mailing list: https://groups.google.com/forum/#!topic/clojure/3yGjDO2YnjQ

It's not possible to use java constants in a case statement. condp = could be used in this case but these are things which could be used in a java switch statement and so it's annoying to give up constant time dispatch. For example:

(case (.getActionMasked event)
MotionEvent/ACTION_POINTER_DOWN :down
MotionEvent/ACTION_UP :up
MotionEvent/ACTION_POINTER_UP :up
MotionEvent/ACTION_MOVE :move
MotionEvent/ACTION_CANCEL :cancel
MotionEvent/ACTION_OUTSIDE :outside
:none))

Doesn't work, but there is no reason this couldn't be resolved at compile time and dispatched in constant time.



 Comments   
Comment by Herwig Hochleitner [ 02/Mar/14 11:32 AM ]

Another solution for this problem: http://dev.clojure.org/jira/browse/CLJ-1368





[CLJ-1347] finalize won't work in reified objects - document Created: 10/Feb/14  Updated: 01/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

java 7



 Description   

Finalize is called for reified objects even when they are still reachable. It gets called second time at proper time.

user=> (def x (reify Object (finalize [o] (println "OH MY!"))))
#'user/x
user=> (System/gc)
nil
OH MY!
user=> x
#<user$reify__1496 user$reify__1496@53fb35af>
user=> (System/gc)
nil
user=> (def x nil)
#'user/x
user=> (System/gc)
nilOH MY!

Deftype seems to work fine

user=> (deftype T [] Object (finalize [o] (println "great success")))
user.T
user=> (def y (->T))
#'user/y
user=> (System/gc)
nil
user=> (def y nil)
#'user/y
user=> (System/gc)
great success


 Comments   
Comment by Alex Miller [ 10/Feb/14 8:38 AM ]

Just a note: the calls to System/gc don't necessarily cause finalizers to run on the first try - sometimes it took more than one for that to succeed for me. You'd think System/runFinalizers would do it but I had no luck at all with that.

Comment by Gary Fredericks [ 13/Feb/14 10:01 PM ]

reify actually creates two objects – the first is created by reify*, and then reify immediately calls with-meta on it, creating a copy.

The docstring sort of describes this behavior: "reify always implements clojure.lang.IObj and transfers meta data of the form to the created object."

Comment by Jozef Wagner [ 14/Feb/14 5:01 AM ]

Oh, so finalizer is a no-no in reify. Should be mentioned in docs IMO.

Comment by Gary Fredericks [ 14/Feb/14 6:28 AM ]

Just for fun you could do something tricksy like:

^::second-object
(reify Object
  (finalize [self]
    (when (::second-object (meta self))
      ...)))

(have not actually run this)

Comment by Gary Fredericks [ 01/Mar/14 1:36 PM ]

It looks like the class generated by reify always has a constructor that takes a metadata argument, so it doesn't seem out of the question to eliminate the extra object altogether.

I'll try to keep digging on this.





[CLJ-1363] Field access via .- in reflective case does not work Created: 18/Feb/14  Updated: 28/Feb/14  Resolved: 28/Feb/14

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: interop

Attachments: Text File clj-1363-v1.patch     Text File clj-1363-v2.patch     Text File clj-1363-v3.patch    
Patch: Code and Test
Approval: Ok

 Description   

The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user=> (. ^T t a)  ;; as expected (prefer method)
"method"
user=> (. ^T t -a) ;; as expected (prefer field)
"field"
user> (. t a)      ;; as expected (prefer method)
"method"
user> (. t -a)     ;; WRONG - should return "field"
"method"

Approach: This case falls into Reflector.invokeNoArgInstanceMember() (this is the only place this method is used). InstanceFieldExpr now takes another flag (requireField) which will be set to true if "-field" and false if "field". InstanceFieldExpr will invoke (or emit) a call to Reflector.invokeNoArgInstanceMember() which now takes the same flag. If the flag is set to true, it first looks only for a field, otherwise it looks for a method and falls back to field which throws an error if necessary. I added a new invokeNoArgInstanceMember() with an arity to match the old arity - existing bytecode compiled on older Clojure versions will be trying to call this arity.

Patch: clj-1363-v3.patch

Screened by:



 Comments   
Comment by Rich Hickey [ 20/Feb/14 7:24 PM ]

You can't change the semantics of invokeNoArgInstanceMember - they are correct when not using '-'. We need to feed the info that '-' was used through InstanceFieldExpr and make field-first conditional on that.

Comment by Alex Miller [ 21/Feb/14 5:42 AM ]

Updated with new patch to thread this case through InstanceFieldExpr.

Comment by Andy Fingerhut [ 28/Feb/14 6:02 AM ]

A patch for this ticket has been committed as part of Clojure 1.6.0-beta2: https://github.com/clojure/clojure/commit/5fda6cb262d1807566ecadd3af9aaafb58ee5544

It appears this ticket could be closed now.





[CLJ-1239] faster, more flexible dispatch for clojure.walk Created: 29/Jul/13  Updated: 27/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 3
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

 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-1353] Prevent test app from appearing in Mac OS X dock Created: 16/Feb/14  Updated: 27/Feb/14  Resolved: 27/Feb/14

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

Type: Enhancement Priority: Trivial
Reporter: Steve Miner Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None
Environment:

Mac OS X


Attachments: Text File CLJ-1353-no-mac-dock.patch     Text File CLJ-1353-v2.patch     Text File clj-1353-v3.patch     Text File clj-1353-v4.patch    
Patch: Code
Approval: Ok

 Description   

During a local ant build of Clojure (tested with master after release of 1.6.0-beta1), the script/run_test.clj is executed. As a side-effect on the Mac, the Java coffee cup app icon is placed in the Dock, and the test app becomes the active application on the desktop. This is slightly annoying.

Even with this property set, activation of awt causes focus to switch temporarily then switch back (at least on Mac).

Solution: Set the following properties during the build:

java.awt.headless=true

Patch: clj-1353-v4.patch



 Comments   
Comment by Steve Miner [ 16/Feb/14 1:39 PM ]

CLJ-1353-no-mac-dock.patch adds a line to script/run_tests.clj to set the apple.awt.UIElement property. This prevents the test app from appearing in the Dock on Mac OS X.

Comment by Steve Miner [ 16/Feb/14 2:18 PM ]

CLJ-1349 might rearrange the affected source, which would force an update to this patch. Still just a one-liner so maybe it could be added to the patch for CLJ-1349.

Comment by Alex Miller [ 16/Feb/14 5:20 PM ]

I also find this highly annoying.

Comment by Andy Fingerhut [ 17/Feb/14 11:21 AM ]

Patch CLJ-1353-v2.patch is identical to Steve Miner's CLJ-1353-no-mac-dock.patch, except it adds another line to build.xml to set the property there, too. At least on my Mac systems, an icon appears in the dock during compilation, not only during testing, and this added line prevents that. Keeps Steve as the patch author.

Comment by Andy Fingerhut [ 17/Feb/14 11:22 AM ]

I tested CLJ-1353-v2.patch on a Linux system, too, and at least the messages that appear on the console during the execution of "ant" are identical with and without this patch, so no extra warnings appear due to these extra properties being set that are likely ignored by the JVM there.

Comment by Steve Miner [ 17/Feb/14 1:45 PM ]

Adding the sysproperty setting to build.xml sounds like a good idea. Thanks.

Comment by Alex Miller [ 18/Feb/14 1:42 PM ]

I found that even with this property, I still see focus change, then come back, during the build due to the activation of awt. Adding the java.awt.headless=true property made that stop. I updated the patch in both locations and now on Mac focus is never stolen during the build.

FYI: If you see the Java "Allow incoming network connections?" dialog on Mac during the tests in response to creating the Sockets in test/clojure/test_clojure/java/io.clj (test-socket-iofactory), this procedure makes that stop:

http://techblog.willshouse.com/2012/10/17/how-to-allow-java-in-the-firewall-on-os-x-mountain-lion/

Beware tracking down the correct version of Java (for example the 1.6 version) instead of the easier to find 1.7 version - the permissions are separate for each version.

Comment by Andy Fingerhut [ 24/Feb/14 2:35 PM ]

In my testing, the addition of the java.awt.headless=true properties in both build.xml and src/script/run_tests.clj was sufficient to avoid the additional icon appearing, and also avoiding any change of focus. Setting apple.awt.UIElement=true appears to be unnecessary (but harmless).

Comment by Steve Miner [ 24/Feb/14 3:28 PM ]

Yes, it seems that java.awt.headless=true is a better, more general solution for the build process. I think apple.awt.UIElement would be appropriate if you actually needed AWT for user interaction but didn't want the dock icon.

Comment by Alex Miller [ 25/Feb/14 11:33 AM ]

Added v4 patch that only sets java.awt.headless=true and drops the apple property.





[CLJ-1355] Restore symbol and keyword hashCode to avoid breaking compiled case expressions Created: 17/Feb/14  Updated: 27/Feb/14  Resolved: 27/Feb/14

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

clojure 1.6.0-beta1


Attachments: File clj-1355-cached.diff     Text File clj-1355-v2.patch    
Patch: Code
Approval: Ok

 Description   

case expressions compiled in Clojure 1.5 are broken if run with Clojure 1.6 where hashCode behavior has diverged from hasheq. In particular, Symbol and Keyword fall into this category.

Approach: Cache both hashCode (with 1.5 calculation) and hasheq (new 1.6 calculation) in Symbol and just hasheq in Keyword. In 1.5, these were the same and case expressions compiled with 1.5 will store the old hash calculation. In 1.6, the hashCode of an expression will be used for comparison.

I tested this by AOT compiling a project in clojure 1.5.1 with this function:

(defn check [v]
  (case v
    :k "keyword match"
    'k "symbol match"
    "k" "string match"
    "no match"))

I verified that (check :k) and (check 'v) incorrectly returned "no match" on Clojure 1.6.0-beta1. I then verified that they returned "keyword match" and "symbol match" respectively on Clojure 1.6.0-master with this patch applied.

Patch: clj-1355-v2.patch



 Comments   
Comment by Alex Miller [ 17/Feb/14 9:38 AM ]

Add patch that caches a new hash field for both Symbol and Keyword that retains Clojure 1.5 computations.

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

There is a concern here that we are adding a new int field to every Symbol and Keyword (and keyword holds a symbol, so it's really 2 for each keyword).

Comment by Rich Hickey [ 20/Feb/14 7:27 PM ]

I don't think we need to cache in keyword, it's just an add

Comment by Alex Miller [ 20/Feb/14 9:24 PM ]

Updated patch to only cache hashCode in symbol and compute in Keyword.





[CLJ-1352] clojure.test/test-vars runs :each fixtures for vars without :test metadata Created: 14/Feb/14  Updated: 25/Feb/14  Resolved: 25/Feb/14

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

Type: Defect Priority: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File tcrawley-fixtures-with-non-test-vars-2014-02-14.diff    
Patch: Code and Test
Approval: Ok

 Description   

The patch for CLJ-866 introduced a bug with :each fixtures and non-test vars: the fixtures are invoked for every var, not just ones with :test metadata.

Patch: tcrawley-fixtures-with-non-test-vars-2014-02-14.diff

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 24/Feb/14 2:37 PM ]

The patch for this ticket has been committed: https://github.com/clojure/clojure/commit/919a7100ddf327d73bc2d50d9ee1411d4a0e8921

but the ticket has not yet been closed.

Comment by Alex Miller [ 24/Feb/14 3:09 PM ]

yeah, I noticed that too. I was going to mention it to Stu the next time we talked.





[CLJ-1354] Make the class APersistentVector.SubVector public Created: 17/Feb/14  Updated: 25/Feb/14  Resolved: 25/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-1354-make-APersistentVector.SubVector-public.patch    
Patch: Code
Approval: Ok

 Description   

The patch marks APersistentVector.SubVector public so that it can be used as a type hint for reflection-free access to subvec internals. I missed this in CLJ-1150.

core.rrb-vector needs access to the internals of the built-in vector types in order to support their efficient concatenation and (true, RRB-style) slicing.

Patch: 0001-CLJ-1354-make-APersistentVector.SubVector-public.patch

Screened by: Alex Miller



 Comments   
Comment by Michał Marczyk [ 17/Feb/14 7:30 AM ]

This is the exact spot where I'm trying to get at SubVector internals in core.rrb-vector:

https://github.com/clojure/core.rrb-vector/blob/core.rrb-vector-0.0.10/src/main/clojure/clojure/core/rrb_vector/rrbt.clj#L976

With 1.6.0-alpha3, {{(fv/catvec (subvec [0 1 2 3] 1 2) [:foo])}} results in IllegalAccessError tried to access class clojure.lang.APersistentVector$SubVector from class clojure.core.rrb_vector.rrbt$eval2476$fn__2477 clojure.core.rrb-vector.rrbt/eval2476/fn--2477 (rrbt.clj:978). With this patch applied, it works as expected, returning [1 :foo].





[CLJ-1359] Fix changelog typos for 1.6 Created: 18/Feb/14  Updated: 25/Feb/14  Resolved: 25/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

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

 Description   

Some reported problems in the 1.6 changelog:

1) two different issues are both called CLJ-935
2) two issues that are probably different are both called CLJ-1328
3) "Make range consistently return () with a step of 0." This is slightly incorrect. Range now consistently returns an infinite sequence of start with a 0 step.

Patch: clj-1359.patch - updated for these issues, may want to hold this and update for any post-beta1 changes too.






[CLJ-1318] Support destructuring maps with namespaced keywords Created: 06/Jan/14  Updated: 23/Feb/14  Resolved: 31/Jan/14

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

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

Attachments: File clj-1318-1.diff     File clj-1318-2.diff     File clj-1318-3.diff     File clj-1318-4.diff     File clj-1318-5.diff     File clj-1318-6.diff    
Patch: Code and Test
Approval: Ok

 Description   

Current :keys destructuring expects symbols and creates local bindings based on those symbols. This works fine with maps that use non-namespaced keyword keys. This enhancement is to add support for destructuring maps with namespaced keyword keys.

;; typical key destructuring for keyword keys without namespaces
(let [{:keys [a b]} {:a 1 :b 2}] (+ a b))

;; WANT some way to destructure map with namespaced keys
(let [{:keys [????]} {:x/a 1 :y/b 2}] (+ a b))

Approach: Allow keywords (with or without namespaces) in :keys destructuring. Destructure to bindings with the name of the keyword (namespace is ignored).

;; this now works
(let [{:keys [x/a y/b]} {:x/a 1 :y/b 2}] (+ a b))

;; add support for putting keywords into :keys as well to support ::keywords
(let [{:keys [:x/a :y/b]} {:x/a 1 :y/b 2}] (+ a b))
(let [{:keys [::a]} {:user/a 1}] a)

;; syms will also now support namespaced symbols
(let [{:syms [x/a y/b]} {'x/a 1 'y/b 2}] (+ a b))

Patch: clj-1318-6.diff

Screened by: Stuart Sierra. See comments, below.

Doc TODO: Will need to update http://clojure.org/special_forms#binding-forms with new binding form.



 Comments   
Comment by Nicola Mometto [ 06/Jan/14 11:58 AM ]

Why {:keys [:a/b]} and not {:keys [a/b}}?
Also, this should probably be extended to :syms for consistency

Comment by Alex Miller [ 06/Jan/14 12:28 PM ]

Good questions both. For the first question, we want to make locally namespaced keywords (::foo) work and there is no way to say that as a symbol.

I am waiting to hear back from Rich whether support for namespaced :syms is desirable. I think the change to support it is identical to the change to support namespaced keywords as symbols. I'm going to proactively update the patch to support that too.

Comment by Alex Miller [ 06/Jan/14 12:50 PM ]

Added new patch - now supports namespaced symbols or keywords in :keys and namespaced symbols in :syms.

Comment by Rich Hickey [ 06/Jan/14 1:00 PM ]

Should (also) support symbols for names, e.g. {:keys [a/b]}, only limitation is you can't get ns alias resolution. :syms support makes sense, but may seem weird to provide keywords for local names (where it doesn't as much for keywords), but would allow reaching aliases. My preference is no keyword names support for :syms, i.e. {:syms [a/b]} ok, {:syms [:a/b]} not.

Comment by Nicola Mometto [ 06/Jan/14 1:10 PM ]

To me {:syms [:a/b]} doesn't feel any more weird than writing {:keys [:a/b]}.
If this is going to be added, I think it should be consistent for :keys and :syms.
I understand that :syms is rarely used and this should not be an issue realistically, but I would expect everything that works for :keys to work for :syms too and adding only half a feature to :syms might cause unnecessary confusion.

Comment by Nicola Mometto [ 07/Jan/14 2:16 PM ]

With this patch this will now work:

user=> (let [:a/b 1] b)
1

I don't think this is desiderable.

Comment by Alex Miller [ 07/Jan/14 3:52 PM ]

Right, that is a consequence of allowing keywords in the :keys. At a glance this seems hard to address without significant changes unless we catch it prior to processing. Will consider.

Comment by Alex Miller [ 07/Jan/14 4:40 PM ]

Added new patch variant that catches keywords as let binding keys and throws an Exception.

Comment by Alex Miller [ 10/Jan/14 2:24 PM ]

Added one test in -4 showing example of auto-resolved keywords in :keys.

Comment by Stuart Sierra [ 10/Jan/14 3:00 PM ]

Screened. A few comments:

1. The examples in the tests use {:keys (a b)} with lists instead of {:keys [a b]} with vectors. Both forms are accepted both before and after the patch, but the docs at Clojure - special_forms only show vectors.

2. I would like this to work, but it would add some complexity:

(ns com.example.myproject.foo)

  (def data
    {::a 1 ::b 2})

  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
  (ns com.example.myproject.bar
    (:require [com.example.myproject.foo :as foo]))

  ;; I would like this to work:
  (let [{:keys [foo/a foo/b]} foo/data]
    [a b])
  ;;=> [nil nil]

  ;; This is good enough, however:
  (let [{:keys [::foo/a ::foo/b]} foo/data]
    [a b])
  ;;=> [1 2]

3. This doesn't produce an error, which is logically consistent but perhaps not desirable:

(let [{:a ::foo/a} foo/data]
    [a])
Comment by Rich Hickey [ 24/Jan/14 10:11 AM ]

please change the tests to use vectors

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

Added new -5 diff that uses vectors instead of lists in :keys tests.

Comment by Alex Miller [ 24/Jan/14 11:07 AM ]

And also fixing :syms [] in -6 diff.

Comment by Alex Miller [ 24/Jan/14 11:08 AM ]

Changed examples in description to use [].

Comment by Fogus [ 07/Feb/14 2:23 PM ]

A potential point of confusion here is illustrated by the following:

(let [m {:x/a 1, :y/b 2, :x/b 3000}
        {:keys [x/a y/b x/b]} m]
  (+ a b))

//=> 3

To get the answer 3001 one needs to remove the conflicting binding :y/b. Maybe this is not a big deal, but expect questions for the next 100 years.

Comment by David Nolen [ 23/Feb/14 5:01 PM ]

Ported to ClojureScript with CLJS-745





[CLJ-1148] adds docstring support to defonce Created: 17/Jan/13  Updated: 20/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: Joe Gallo Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: docstring

Attachments: Text File 0001-new-defonce-hotness.patch     Text File clj-1148-defonce-2.patch     Text File clj-1148-defonce-3.patch     Text File defonce_fixes.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Pass all args from defonce on to def so it supports docstrings (or potentially other future features) just like def.

Docstrings and other Var metadata will be lost when the defonce is reëvaluated.

Patch: clj-1148-defonce-3.patch

Screened by: Stuart Sierra



 Comments   
Comment by Alex Miller [ 29/Aug/13 9:53 AM ]

Changed to defect for stomping metadata.

Comment by Stuart Halloway [ 18/Oct/13 8:00 AM ]

Please add tests. The clojure.test-helper namespace has useful temporary namespace support.

Comment by Joe Gallo [ 24/Oct/13 12:44 PM ]

This new patch includes the changes to defonce and also tests.

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

Changing to Vetted so this is screenable again.

Comment by Rich Hickey [ 22/Nov/13 11:31 AM ]

I disagree about the stomp metadata - different metadata was provided. The purpose of defonce is to avoid the re-evaluation of the init. Is this the simplest change that accomplishes the doc string? In any case split in two.

Comment by Alex Miller [ 29/Dec/13 10:24 PM ]

Reduced scope of ticket to just passing defonce args on to def to add support for docstring. Added new patch that does this.

Comment by Stuart Sierra [ 10/Jan/14 4:09 PM ]

Screened clj-1148-defonce-2.patch but returning to 'incomplete' status.

The :arglists metadata in this patch (a list of symbols) is inconsistent with all other uses of :arglists (a list of vectors).

Other than that the patch is good.

Comment by Alex Miller [ 10/Jan/14 5:04 PM ]

Updated patch to address inconsistency in arglist format and attached clj-1148-defonce-3.patch.

Comment by Stuart Sierra [ 17/Jan/14 9:36 AM ]

The patch clj-1148-defonce-3.patch is OK but it doesn't really address the docstring issue because defonce still destroys metadata. For example:

user=> (defonce foo "docstring for foo" (do (prn 42) 42))
42
#'user/foo
user=> (doc foo)
-------------------------
user/foo
  docstring for foo
nil
user=> (defonce foo "docstring for foo" (do (prn 42) 42))
nil
user=> (doc foo)
-------------------------
user/foo
  nil
Comment by Stuart Sierra [ 17/Jan/14 10:03 AM ]

Screened with reservations noted.

Comment by Rich Hickey [ 24/Jan/14 10:15 AM ]

Stuart is right, second defonce should retain the doc string (since it again provides it, should be no-op)

Comment by Alex Miller [ 20/Feb/14 10:41 AM ]

pull out of 1.6





[CLJ-1362] Reduce broken on some primitive vectors Created: 18/Feb/14  Updated: 19/Feb/14

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

Type: Defect Priority: Major
Reporter: Nathan Davis Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File clj-1362-v1.patch    
Patch: Code and Test
Approval: Triaged

 Description   

In some cases, reduce over a sequence from a primitive vector created with vector-of will return incorrect answers:

user=> (into [] (drop 32 (into [] (range 33))))
[32]
user=> (into [] (drop 32 (into (vector-of :int) (range 33))))
[0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32]

Second call should return [32] just like the first one.

Cause: VecSeq (seq on primitive Vec obtained with vector-of) maintains two flags: i is the total number of elements prior to the current node in this seq. offset is the offset in the current anode. When using internal-reduce on a VecSeq, the starting index for the reduce was using offset and ignoring i.

Solution: Use (+ i offset) as the starting index.

Patch: clj-1362-v1.patch

Screened by:



 Comments   
Comment by Alex Miller [ 18/Feb/14 10:18 PM ]

We did some debugging on this at the St. Louis Clojure Meetup tonight and suspect the problem is happening when drop walks through the chunked seq over the vector. Specifically, in the VecSeq's implementation of IChunkedSeq.chunkedNext() at https://github.com/clojure/clojure/blob/master/src/clj/clojure/gvec.clj#L116 particularly the offset 0 at the end.

Comment by Alex Miller [ 19/Feb/14 2:41 PM ]

Upon further review, the VecSeq seems to be created properly during chunking. The real issue is in internal-reduce where the starting index is improperly computed.





[CLJ-1364] Primitive VecSeq does not implement equals or hashing methods Created: 19/Feb/14  Updated: 19/Feb/14

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