I just realized (with the asssistance of Paul Stadig) that just doing only longs is probably sufficient, as you can get the integer version if you really want it:

> (int (bit-and Integer/MAX_VALUE (unsigned-bit-shift-right -5 1)))
2147483645

Of course, that's less efficient than just doing it directly with java, but it's enough that I think my concern from the previous comment is addressed.

I have attached "0001-CLJ-827-Add-bit-shift-right-logical.patch", which implements a logical bit-shift-right using the same JVM bytecode as >>>.

The patch mimics the implementations of << and >>.

For context, this feature appears to be needed for Clojure-in-Clojure data structures: https://groups.google.com/d/msg/clojure-dev/iAwH7CLSFzE/6wzDH4RS1YQJ

Just wanted to note that I've introduced this operation to ClojureScript when implementing PersistentHashMap. The name over there is bit-shift-right-zero-fill. Would it be alright for Clojure to use that name? Failing that, ClojureScript would probably have to change to match.

I've added clj-827-unsigned-bit-shift-right-with-tests.patch which builds off of Tim's patch and adds tests. I also renamed the new fn to unsigned-bit-shift-right after chatting with Rich.

@Michal - This may mean you need to rename the cljs fn. After this lands on master, I can open a ticket with a patch if you'd like.

Converted from http://www.assembla.com/spaces/clojure/tickets/322
Attachments:
aot-transitivity-option-compat-322.diff - https://www.assembla.com/spaces/clojure/documents/aI7Eu-HeGr35ImeJe5cbLA/download/aI7Eu-HeGr35ImeJe5cbLA
aot-transitivity-option-322.diff - https://www.assembla.com/spaces/clojure/documents/aIWFiWHeGr35ImeJe5cbLA/download/aIWFiWHeGr35ImeJe5cbLA

hlship said: I'd like to reinforce this. I've been doing research on Clojure build tools for an upcoming talk and all of them (Maven, Leiningen, Gradle) have the same problem: the AOT compile extends from the desired namespaces (such as one containing a :gen-class) to every reached namespace. This is going to cause a real ugliness when application A uses libraries B and C that both depend on library D (such as clojure-contrib) and B and C are thus both bloated with duplicate, unwanted AOT compiled classes from the library D.

cemerick said: This behaviour is an implementation detail of Clojure's AOT compilation process, and is orthogonal to any particular build tooling.

I am working on a patch that would provide a mechanism for such tooling to disable this default behaviour.

cemerick said: A first cut of a change to address this issue is here (caution, work in progress!):

http://github.com/cemerick/clojure/commit/6f14e0790c0d283a7e44056adf1bb3f36bb16e0e

This makes available a new recognized system property, clojure.compiler.transitive, which defaults to true. When set/bound to false (i.e. -Dclojure.compiler.transitive=false when using clojure.lang.Compile), only the first loaded file (either the ns named in the call to compile or each of the namespaces named as arguments to clojure.lang.Compile) will have classfiles written to disk.

This means that this compilation invocation:

java -cp <your classpath> -Dclojure.compiler.transitive=false clojure.lang.Compile com.bar com.baz

will generate classfiles only for com.bar and com.baz, but not for any of the namespaces or other files they load, require, or use.

The only shortcoming of this WIP patch is that classfiles are still generated for proxy and gen-class classes defined outside of the explicitly-named namespaces. What I thought was a solution for this ended up breaking the loading of generated interfaces (as produced by defprotocol, etc).

I'll take a second look at this before the end of the week, but wanted to get this out there so as to get any comments people might have.

technomancy said: Looks good, but I'm having trouble getting it to work. I tried compiling from master of Chas's fork on github, but I still got the all the .class files generated with -Dclojure.compiler.transitive=false. It could be a quirk of the way I'm using ant to fork off processes though. Is it possible to set it using System/setProperty, or must it be given as a property on the command-line?

cemerick said: Bah, that's just bad documentation. :-/

The system property is only provided by clojure.lang.Compile; the value of it drives the binding of clojure.core/transitive-compile, which has a root binding of true.

You should be able to configure the transitivity the same way you configure compile-path (system prop to clojure.lang.Compile or a direct binding when at the REPL, etc).

If not, ping me in irc or elsewhere.

meikelbrandmeyer said: I think, excluding parts 'load'ed is a little strong. I have some namespaces which load several parts from different files, but which belong to the same namespace. The most prominent example of such a case is clojure.core itself. I'm find with stopping require and use, but load is a bit too much, I'd say.

technomancy said: Chas: Thanks; will give that a go.

Meikel: Do people actually use load outside of clojure.core? I thought it was only used there because clojure.core is a "special" namespace where you want more vars to be available than can reasonably fit in a single file. Splitting up a namespace into several files is quite unadvisable otherwise.

technomancy said: I can confirm that this works for me modulo the proxy/gen-class issue that Chas mentioned. I would love to see this in Clojure 1.2; it would really clean up a lot of build-related issues.

meikelbrandmeyer said: I used it several times and this is the first time, I hear that it is unadvisable to do so. Even with a lower number of Vars in the namespace (c.c is here certainly exceptional) and might be of use to split several "sections" of code which belong to the same namespace but have different functionality. Whether to use a big comment in the source to indicate the section or split things into subfiles is a matter of taste. But it's a perfectly reasonable thing todo.

Another use case, where I use this (and c.c.lazy-xml, IIRC) is to conditionally load code depending on whether a dependency is available or not. Eg. vimclojure uses c.c.pprint and c.c.stacktrace/clj-stacktrace transparently depending on their availability.

There are perfectly legal uses of load. I don't see any "unadvisable" here.

cemerick said: Thanks, Meikel; I had forgotten about that use case, as I don't use load directly myself at all. I probably wouldn't say it's inadvisable, just mostly unnecessary. In any case, that's a good catch. It complicates things a bit, but we'll see what happens. I'm going to take another whack at resolving the proxy/gen-class case and narrowing the impact of nontransitivity to use and require later tonight.

I agree wholeheartedly that this should be in 1.2, assuming the technical bits work out. This has been an irritant for quite a long time. I actually believe that nontransitivity should be the default – no one wants or expects to have classfiles show up for dependencies show up when a project is AOT-compiled. I think the only negative impact would be whoever still fiddles with compilation at the REPL, and doesn't use maven or lein – and even then, it's just a matter of binding another var.

meikelbrandmeyer said: Then the var should be added to the default bindings in the clojure.main repl. Then it's set!-able like the other vars ��� warn-on-reflection and friends.

cemerick said: This is looking pretty good (still WIP):

http://github.com/cemerick/clojure/commit/fedfb022ecef420a932b3d69c182ec7a8e5960a6

Thank you again for mentioning load, Meikel: it was very helpful in resolving the proxy/gen-class issue as well.

Just a single data point: the jar produced by the medium-sized project I've been using for testing the changes has shrunk from 1.8MB to less than 1MB. That's not the only reason this is a good change, but it's certainly a nice side-effect.

cemerick said: [file:aIWFiWHeGr35ImeJe5cbLA]

cemerick said: [file:aI7Eu-HeGr35ImeJe5cbLA]

cemerick said: Patched attached. The compat one retains the current default behaviour [*transitive-compile* true], the other changes the default so that transitivity is a non-default option. At least of those I've spoken to about this, the latter is preferred.

The user impact of changing the default would be:

1. The result of compiling from the REPL will change. Getting back current behaviour would require adding a [*transitive-compile* true] binding to the existing bindings one must set when compiling from the REPL.
2. The same as #1 goes for those scripting AOT compilation via clojure.lang.Compile as well (whether by shell scripts, ant, etc).
3. Those using lein, clojure-maven-plugin, gradle, and others will likely have a new option provided by those tools, and perhaps a different default than the language's. I suspect those using such tools would much prefer a change from the default behaviour in any case.

hlship said: Just had a brain-storm:

How about an option to support transitive compilation, but only if the Clojure source file being compiled as a file: URL (i.e., its a local file on the file system, not a file stored in a JAR). That would make it easier to use compilation on the local project without transitively compiling imported libraries, such as clojure-contrib.

So transitive-compile should be a keyword, not a boolean, with values :all (for 1.1 behavior), :none (to compile only the exact specified namespaces) or :local (to compile transitively, but only for local files, not source files from JARs).

cemerick said: (Crossposted to the clojure-dev list)

I thought about this some, and I don't think that's a good idea, at least for now. I'm uncomfortable with semantics changing depending upon where code is being loaded from – which, depending upon a tool's implementation, might be undefined. E.g. if the com.foo.bar ns is available in source form in one directory, but as classes from a jar, and classpaths aren't being constructed in a stable fashion, then the results of compilation will change.

If we decide that special treatment depending upon the source of code is warranted in the future, that's a fairly straightforward thing to do w.r.t. the API – we could have :all and :local as you suggest, with nil representing :none.

stu said: Rich is not comfortable enough with the implementation complexity of this patch (e.g. the guard clause for proxies and gen-class) to slide this in as a minor fix under the wire for 1.2.

Better to live with the pain we know a little longer than ship something we don't have enough experience with to be confident.

Updated patch to cleanly apply to HEAD and address issues raised by screening done by Cosmin Stejerean. Also includes proper tests.

Note: this patch's tests require the fix for CLJ-432!

the "-resolved" patch resolves a conflict in main.clj

Several questions:

1. I am getting an ant build error: "/Users/stuart/repos/clojure/build.xml:137: java.io.FileNotFoundException: Could not locate clojure/test_clojure/aot/nontransitive__init.class or clojure/test_clojure/aot/nontransitive.clj on classpath:"
2. It feels icky to have a method named writeClassFile that, under some circumstances, does not write a class file, but instead loads it via a dynamic loader. Maybe this is just a naming issue.
3. Are there any other ways to accomplish the goals of load-level? Or, taking the other side, if we are going to have a load-level, are there other possible consumers who might have different needs?
4. (Minor) Why the use :only idiom instead of just require?

An alternative approach: patch write-classes-1.diff.gz

From my forked branch

What this patch does:

• Keeps 'compile' and 'compile-files' exactly the same
• Adds 'compile-write-classes' to write .class files for specifically named classes
• Minor compiler changes to support this

This approach was prompted by the following observations:

• Java interop is the dominant reason for needing .class files
• Things other than namespaces can generate classes for Java interop:
  • deftype/defrecord
  • defprotocol
  • gen-class/gen-interface
• For library releases, we want to control which .class files are emitted on a per-class basis, not per-namespace
• Some legitimate uses of AOT compilation will want transitive compilation
  • Pre-compiling an entire application before release

S. Halloway: My apologies, I didn't know you had commented. I thought that, having assigned this issue to myself, I'd be notified of updates.

FWIW, I aim to review your comments and SS' approach over the weekend.

S. Halloway:

1. Certainly shouldn't happen. AFAIK, others have screened the patch, presumably with a successful build.
2. Agreed; given the approach, I think it's just a bad name.
3. Yes, I think S. Sierra's is one. See my next comment.
4. Because the :use form was already there. I've actually been using that form of :use more and more; I've found that easier than occasionally having to shuffle around specs between :use and :require. I think I'm aping Chris Houser in that regard.

I think S. Sierra's approach is fundamentally superior what I offered. I have two suggestions: one slight perspective change (which implies no change in the actual implementation), and an idea for an even simpler approach (at least from a user perspective), in that order.

While interop is the driving requirement behind AOT, I absolutely do not want to have to keep an updated enumeration of all of the classes resulting from each and every defrecord et al. usages in my pom.xml/project.clj (and I wouldn't wish the task of ferreting those usages and their resulting classnames on any build tool author).

Right now, *compile-write-classes* is documented to be a set of classname strings, but could just as easily be any other function. *compile-write-classes* should be documented to accept any predicate function (renamed to e.g. *compile-write-class?*?). There's no reason why it shouldn't be bound to, e.g. #(re-matches #"foo\.bar\.[\w_]+$" %) if I know that all my records are defined in the foo.bar namespace.

To go along with that, I think some package/classname-globbing utilities along with corresponding options to clojure.lang.Compile would be most welcome. Classname munging rules are not exactly obvious, and it'd be good to make things a little easier for users in this regard.

Another alternative

If there's a closed set of forms that generate classes that one might reasonably be interested in having in a build result (outside of use cases for pervasive AOT), then why not have a simple option that only those forms utilize? gen-class and gen-interface already do this, but reusing the all-or-nothing *compile-files* binding; if they keyed off of a binding that implied a diminished scope (e.g. *compile-interop-forms* – which would be true if *compile-files* were true), then they'd do exactly what we wanted. Extending this approach to deftype (and therefore defrecord) should be straightforward.

An implementation of this would probably be somewhat more complicated than S. Sierra's patch, though not as complex as my original stab at the problem (i.e. no *load-level*). On the plus side:

1. No additional configuration for users or implementation work for build tool authors, aside from the addition of the boolean diminished-scope AOT option
2. Class file generation would remain opaque from a build process standpoint
3. Future/other class-generating forms (there are a few people futzing with ASM independently, etc) can make local decisions about whether or not to participate in interop-centric classfile generation. This might be particularly helpful if a given form emits multiple classes, making the determination of a classname-based filter fn less straightforward.

I can see wanting to further restrict AOT to specific classnames in certain circumstances, in which case the above and S. Sierra's patch might be complimentary.

I like the idea of *compile-interop-forms*. But is it always possible to determine what an "interop form" is? I think it is, I'm just not sure.

I'm also in favor of compile-interop-forms. As far as determining, how about sticking metadata on the var?

(defmacro ^{:interop-form true} deftype ...)

Summary and design discussion on wiki at http://dev.clojure.org/display/design/Transitive+AOT+Compilation

New attachment compile-interop-1.patch has new approach: Add a third possible value for *compile-files*. True and false keep their original meanings, but :interop causes only interop-related forms to be written out as .class files. "Interop forms" are gen-class, gen-interface, deftype, defrecord, defprotocol, and definterface.

Pros:

• doesn't change existing behavior
• handles common case for non-transitive AOT (interop)
• minimal changes to the compiler

Cons:

• not flexible

Just realized my patch doesn't solve the transitive compilation problem. If library A loads library B, then compiling interop forms in A will also emit interop .class files in B.

It's disappointing to see an important issue like this still unresolved after 2.5 years. This is a real pain for us. We have a large closed source project where shipping source is not an option. This forces us to manage the AOT'ing of dependencies due to the hard dependency on protocol interfaces introduced by transitive AOT compilation (see https://groups.google.com/forum/?fromgroups=#!topic/clojure-dev/r3A1JOIiwVU).

Paul, do you have a suggestion for which of the approaches described in comments here, or on the wiki page http://dev.clojure.org/display/design/Transitive+AOT+Compilation would be preferable solution for you? Or perhaps even a patch that implements your preferred approach?

Andy,

I'm now consulting with Paudi's organization, so I think I can speak for him (I'm now the default buildmeister).

I like Stuart's :interop idea, but that is somewhat orthogonal to our needs.

I return to what I would like; compilation would compile specific namespaces; dependencies of those namespaces would not be compiled.

To be honest, I'm still a little hung up on the interop forms: especially defprotocol and friends; from a cursory glance, it appears that todays AOT compilation will compile the protocol into a Java class, then compile the namespace that references the protocol with the assumption that the protocol's Java class is available. When we use build rules to only package our namespace's class files into the output JAR, the code fails with a NoClassDefFoundError because the protocol really needs to be recompiled, at runtime compilation, into an in-memory Java class.

Obviously, supporting this correctly will be a challenge; the compiled bytecode for our namespace would ideally:
1) check to see if the Java class already exists and use it if so
2) load the necessary namespaces so as to force the creation of the Java class

I can imagine any number of ways to juggle things to make this work, so I won't suggest a specific implementation.

In the meantime, our workaround is to create a "stub" module as part of our build; it simply requires in the necessary namespaces (for example, org.clojure:core.cache); this forces an AOT compile of the dependencies and we have a special rule to package such dependencies in the stub module's output JAR. This may not be a scalable problem, and it is expensive to identify what 3rd party dependencies require this treatment.

I am marking this incomplete because there does not yet seem to be plurality, much less consensus or unanimity, on approach.

Personally I am in favor of a solution based on a predicate that gets handed the class name and compiled bits, and then can choose whether to write the class. Pretty close to Stuart Sierra's compile-write-classes. Might be possible to flow more information than classname to the predicate.

Here is a workaround patch which makes this error less likely to occur.

Edward, it is Rich Hickey's policy only to consider for inclusion in Clojure patches written by people who have signed a Contributor Agreement: http://clojure.org/contributing

Were you interested in becoming a contributor?

Sure, although the patch attached is emphatically not the one you want to actually applying, since it only band-aids the problem.

I am not sure, but this ticket may be related to CLJ-1171. At least, there the issue was a global name not being shadowed by a local name bound with let. That seems similar to this issue.

Please bring this up on clojure-dev. We'll be able to vet this ticket after that.

Should I respond to my original clojure-dev post about this (linked in the issue description above), or start a new one?

Patch clj-1074-read-infinity-and-nan-patch-v2.txt dated May 24 2013 is identical to 0001-Read-Infinity-and-NaN.patch dated Sep 21 2012, except it applies cleanly to latest master. The older patch conflicts with a recent commit made for CLJ-873.

Patch instead of linking to github

Patch clj-669-use-java.nio-in-do-copy-for-files-patch-v2.txt dated May 24 2013 is identical to patch 0001-use-java.nio-in-do-copy-method-for-Files.patch dated Nov 27 2010 except it applies cleanly to latest master. The older patch conflicts with a recent commit for CLJ-1072 that updates the obsolete #^ syntax for metadata.

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.

CLJ-850-test.patch added.

When the compiler tries to generates the call to the correct overload of invokePrim, it's failing to take the return type into account. I should be calling invokePrim(Ljava/lang/Object;J)J;.

XXX this is where I got myself confused. The invokePrim overload it's trying to invoke is the correct one. But, that apparently is no the one that's being generated. Sorry for the noise.

Here's what I think I'm seeing:

HostExpr.Parse.parse() loses track of the return type, in the final else branch where method calls are handled. This is because tagOf(form), where form is something like: (. foo invokePrim 1) returns nil. (The form itself doesn't have a :tag, but I believe foo does, though that's the name of the appropriate invokePrim interface (i.e. IFn$OLL).

new InstanceMethodExpr(...) then gets constructed with tag==null, at which point we've already lost sine InstanceMethodExpr can't correctly consider overloading on the result type if it doesn't know what it is.

It's not yet clear to me how I can get InstanceMethodExpr to consider the return type, if it knew it...

There are two things going on here. I'm not sure which is the error.

It looks like the return type of the generated invokePrim method is too specific. It's generated as returning String, though the IFn$LO interface specifies returning Object.

The caller attempts to call invokePrim returning Object, which is what the interface IFn$LO specifies, but this doesn't work because methodSL doesn't actually implement that method. Instead it implements an overload returning String.

1. methodSL.invokePrim is declared as (long)->String
2. methodSL.invoke does invokeinterface with the correct return type WRT methodSL, but the wrong return type WRT the IFn$LO interface.
3. callSL.invoke does invokeinterface with the wrong return type WRT methodSL, but the correct return type WRT IFn$LO. (This is the failure we observe in the clj-850 unit test.)

(defn methodSL  ^String [^long i] (str i))

<<1>> public final java.lang.String invokePrim(long);  <<1>>
      Code:
       0:   getstatic   #25; 
            //Field const__0:Lclojure/lang/Var;
       3:   invokevirtual   #34; 
            //Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
       6:   checkcast   #36; 
            //class clojure/lang/IFn
       9:   lload_1
       10:  invokestatic    #42; 
            //Method clojure/lang/Numbers.num:(J)Ljava/lang/Number;
       13:  invokeinterface #46,  2; 
            //InterfaceMethod clojure/lang/IFn.invoke:(Ljava/lang/Object;)Ljava/lang/Object;
       18:  checkcast   #48; 
            //class java/lang/String
       21:  areturn

      public java.lang.Object invoke(java.lang.Object);
      Code:
       0:   aload_0
       1:   aload_1
       2:   checkcast   #54; 
            //class java/lang/Number
       5:   invokestatic    #58; 
            //Method clojure/lang/RT.longCast:(Ljava/lang/Object;)J
<<2>>  8:   invokeinterface #60,  3; 
            //InterfaceMethod clojure/lang/IFn$LO.invokePrim:(J)Ljava/lang/String;
       13:  areturn

(defn callSL ^String [] (methodSL 42))

    public java.lang.Object invoke();
      Code:
       0:   getstatic   #25; 
            //Field const__0:Lclojure/lang/Var;
       3:   invokevirtual   #43; 
            //Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
       6:   checkcast   #45; 
            //class clojure/lang/IFn$LO
       9:   ldc2_w  #26; 
            //long 42l
<<3>>  12:  invokeinterface #49,  3; 
            //InterfaceMethod clojure/lang/IFn$LO.invokePrim:(J)Ljava/lang/Object;
       17:  areturn

Given P is some primitive type, O is type Object, and R some subclass of Object:

When Clojure generates a R invokePrim(P x), it also generates a Object invoke(Object x), which delegates to R invokePrim(P x).

R invokePrim(P x) overloads, but does not override the method of the corresponding Fn$PO interface.

If Clojure were to generate an additional O invokePrim(P x) which delegates to R invokePrim(P x), it would satisfy the requirements of the Fn$PO interface, and should fix this issue.

CLJ-850.patch fixes the issue.

I consider this patch to be pretty hackish and hope that there's a cleaner way of addressing CLJ-850. This is the first time I've tried to understand (much less change) the Clojure compiler, so don't expect genius.

The patch lies slightly:

Clojure needs to generate an additional O invokePrim(P x) method to
satisfy the interface. This also delegates to R invokePrim(P x).

It turns out that what I'm actually doing is generating a R invokePrim(P x) which is a copy of O invokePrim(P x), instead of delgating to O invokePrim(P x). This works, but the resulting class file would be smaller if the patch actually did what it says it does.

clj-850-type-hinted-fn-abstractmethoderror-patch2.txt is identical to Ben's two patches combined into one, with the small modification that the new tests are added to metadata.clj instead of creating a new test file. The patch applies cleanly to latest master as of Feb 27, 2012. One of the new tests does fail without the change to the compiler, and succeeds with it. I can't vouch for the correctness of the change myself, not knowing enough about the compiler internals to judge.

Same comments as made on Feb 27, 2012, except the patch clj-850-type-hinted-fn-abstractmethoderror-patch3.txt applies cleanly to latest master as of Mar 23, 2012. Updated because previous patch (now removed) no longer applied cleanly. git patches often fail to apply if context lines near changes are modified.

We don't support sigs taking prims and returning anything other than prim or Object. Overloading on return value only is a bad idea (and forbidden in Java). The return type of the generated method should be Object, and the String return hint should be used only as a hint.

clj-850-type-hinted-fn-abstractmethoderror-patch4.txt dated Nov 1 2012 is same as Ben Smith-Mannschott's CLJ-850.patch and CLJ-850-test.patch, except it has been combined into one patch and does not create a new test source file.

+10 for solving this issue: it keeps biting me in 1.4 and wouuld love to see in 1.5

I'm not familiar with the Clojure compiler internals, but looking at the approach, shouldn't we produce a primitive method with a different name (since Java doesn't support overloading on return types as Rich correctly points out). Also I think there should be 4 methods:

R invokePrimExact(P x) - the actual method, used when compiler can infer
R invokePrimExact(O x) - delegates, used when compiler can't infer type of x
Object invokePrim(P x) - primitive method, conforms to IFn$PO interface, delegates
Object invoke(Object x) - general method, delegates

I think this solves all the important cases?

Still no patch incorporating my feedback, afaict. Pushing to next release.

Does this new patch address the issue and concerns? (This incorporates Ben's tests from the previous patch, wasn't sure how to attribute him on that hunk) CLJ-850-conform-to-invokePrim.diff

Presumptuously changing state from Incomplete back to Vetted after Ghadi Shayban added the patch CLJ-850-conform-to-invokePrim.diff dated Dec 19 2012 after the status was changed to Incomplete.

Patch clj-1197-make-bigint-work-on-all-doubles-v1.txt dated Apr 7 2013 changes bigint and biginteger so that they return the correct value for all float and double values, and no longer throw an exception.

Looks good. Tests pass and the failing example now converts correctly

This patch works great for me to avoid OOM/PermGen crashes from classloaders being retained [mine is a non-servlet use case].

Does Tomcat create warnings for Clojure, as described e.g. here?

If so, does this patch make the warnings go away?

Stu: that's a good question. I'll take a look at Tomcat this afternoon.

The code that calls transaction.remove() seems unncessarily subtle. There are two exits from the method, and only one is protected by the finally block.

If the "outer" case was a top-level if, the logic would be more clear, and only the "outer" case would need try/finally, which might reduce the performance penalty in the case of deeply nested dosyncs.

Did your transaction overhead of 4-6 microseconds test only one level of dosync, or many?

Because the unwind code calls remove at the top (as opposed to set(null)), the code should now be safe for use with Clojure-defined ThreadLocal subclasses.

Therefore, Var's use of an initialValue should be irrelevant to this patch, and it should be possible to fix this bug with a patch half the size of the current patch, touching only LockingTransaction.runInTransaction and Var.popThreadBindings.

Fix for this defect.

The throwArity now attempts to locate and call clojure.main/demunge. If it finds the function it invokes it and uses the returned string in the error. Otherwise it just throws the actual class name. This results in the following behaviour:

user=> (defn a->b [])
#'user/a->b
user=> (a->b 32)
ArityException Wrong number of args (1) passed to: user/a->b clojure.lang.AFn.throwArity (AFn.java:449)
user=>

Timothy: Why the empty catch block? I don't see anything in the try block whose failure we would want to ignore.

Attached patches test and fix issue + tangent

Note: Patch 0003 just adds the arity check, hence is optional, but if it's omitted from the patchset, the corresponding test from patch 0001 will fail.

Summarizing the decisions in these patches:

• instance? and apply instance? should be consistent
• they should check arity (matching apply instance? existing behavior)
• they should allow lexical shadowing of the class argument (matching apply instance? existing behavior)

It is possible (although unlikely) that existing code relies on the current eccentric behavior of instance?. I think it would be fair to categorize programs relying on this behavior as buggy, but that is easy for me to say.

Notes from the dev mailing list:

The "sources" JAR is generated by another Maven plugin, configured here:
https://github.com/clojure/clojure/blob/clojure-1.5.0-RC15/pom.xml#L169-L181

The simplest solution might be to just exclude the file from the sources jar. It looks like maven-source-plugin has an excludes option which would do the trick:

http://maven.apache.org/plugins/maven-source-plugin/jar-mojo.html#excludes

The attached patch just binds *read-eval* to true inside source-fn.

Note: Allowing this implies that you trust the data on your classpath. If there are reasons somebody might not, we should reject this patch and people will have to be explicit when calling source.

Ugh, that's a fair point when it comes to sandboxing. I'll check with the owners of clojurebot and lazybot.

I haven't come up with any scenarios where this is problematic, and I haven't heard anything back from the bot owners. As for sandboxing, clojure.repl can easily be excluded.

Looks good

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}

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

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.

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

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

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

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

Looks good

Clojure 1.3 om Mac OS 10.7, Clojure 1.5.0 alpha1 on Linux x86_64 (OpenJDK 1.7.0 b147)

The attached patch fixes the issue, by emitting IPersistentMap instead of Persistent{Hash|Array}Map as class type for maps literals

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.

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?

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

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.

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.

I just realized that my patch also implements CLJ-1086.

Would be nice if tests also demonstrated that metadata is preserved correctly.

New patch in response to stuarthalloway feedback.

This patch also prevents an infinite loop in the macroexpander when fed the following expression:

(->> a b (->> c d))

Edit: Far simpler example.

I have signed the CA and it is in the mail.

My CA has now been applied. This patch is quite simple – can someone have a look at it please?

FYI, I have run into this in actual code as well (implementing a query language function library).

clj-873-namespace-divides-patch.txt is same as Chris's, just updated to apply cleanly to latest master as of Feb 24, 2012.

The test he added does fail without the code fix, and passes with it. He is now on the list of contributors.

A short further discussion of this patch appeared here: http://groups.google.com/group/clojure-dev/browse_thread/thread/f095980802a82747/b723726c77c1ec64

Also, I assume this bug is what is referred to in Clojurescript's core.cljs, where it says ";; FIXME: waiting on cljs.core//"

Thanks all. It is nice to have supporting real-world stories such as Alex's in the comments.

I should have added a comment here a while back if it would have helped, but David Nolen's CLJ-930 was closed as a duplicate of this one.

This also affects a two of my libraries: 1) CSS generation library I'm working on, which wants to be able to do division with pixels and other units. 2) Factjor which defines binary math operators against a stack.

clojure.lang.EdnReader should get patched aswell.

Please change this to link to the definitive prose, so we don't have to maintain that in two places.

Feel free to correct the wording, I used something simple.

Stu, he linked to clojure.org as you requested so I'm moving this along.

Modified this ticket to cover all remaining cases of old metadata syntax. Added patch.

All platforms that provide xdg-open (as part of freedesktop.org) benefit from this. Fix was tested on OpenBSD.

CLJ-920, if not identical, at least bears a significant resemblance to this ticket. It would be good to see if the patch for one of them fixes both issues.

clj-896-browse-url-uses-xdg-open-patch2.txt is based more on the patch attached to CLJ-920 by Jeremy Heiler than on the earlier patch attached to this ticket. He and I have signed CAs.

I think this patch improves on both of the previous patches for CLJ-896 and CLJ-920. In particular, Jeremy's worked fine, but it caused a long slowdown in the running of tests when building Clojure. This one does not.

Tested on:

Mac OS X 10.6.8
Windows XP SP3, both in cmd.exe and a Cygwin bash shell
Ubuntu 10.04 LTS

It would be great if someone could test it on a BSD system. The only possible issue I can think of is whether the output of the "which" command is different there than on the Linux system I tested.

If someone wants to make a patch that doesn't use "which", but instead checks the PATH, I'd recommend they also test on Windows in cmd.exe to make sure it works correctly there.

Screened. Verified on Mac OS X.

And I've tested it on OpenBSD.

Any

Created a properly formatted patch, attached, for Jason's enhancement. I tested it with

(inspect-tree (:members (clojure.reflect/reflect java.lang.Math)))

and it worked, whereas it had many errors without Jason's changes.

Jason Wolfe has signed a CA. Patch applies cleanly with latest master as of Feb 14, 2012. No errors, warnings, or test failures with the patch applied. No doc strings need updating.

Screened.

(lazy-seq nil) should be ()

Hey Rich, if you're talking about the first line of the diff:

+ ([] (lazy-seq nil))

Then that's the implementation, not the tests – given an empty vector of arguments, return (lazy-seq nil), which I just copied from the existing definition of concat.

Cheers,
Joe

(defn interleave [& s] (apply mapcat list s))

Marc's definition doesn't work for no arguments. Maybe:

(defn interleave
([] ())
([& s] (apply mapcat list s)))

Yes, but my solution is too slow, as it uses "apply".

Patch clj-863-make-interleave-handle-odd-args-like-concat-patch-v1.txt dated Jan 1 2013 is identical to Joe Gallo's 0001-make-interleave-handle-odd-arugments-in-the-same-man.patch patch dated Oct 24 2011, except it returns () instead of (lazy-seq nil) as per Rich's comment. If concat should also return () instead of (lazy-seq nil), perhaps another ticket should be created to fix that. Also presumptuously changing ticket state from Incomplete back to Vetted, since the reason it was marked Incomplete should now be addressed, and it was Screened before.

clj-1143-ns-doc-string-correction-v1.txt dated Jan 10 2013 replaces (refer 'clojure) with (refer 'clojure.core) in the doc string of the ns macro.

Add *default-data-reader-fn* to the special bindings in main.clj so that it is set!-able in the REPL

This is a one-liner that makes *default-data-reader-fn* convenient to use in the REPL, similar to how *data-readers* works. I'd like to have this fix in Clojure 1.5.

work-around for REPL:

(alter-var-root #'clojure.core/*default-data-reader-fn* (constantly my-default-reader))

clj-908-Print-metadata-and-anonymous-classes-better-patch2.txt dated May 19, 2012 only has context line changes from the previous one, 0001-Print-metadata-and-anonymous-classes-better.patch dated Jan 10, 2012. The previous one no longer applies cleanly to the latest master, while the new one does.

Screened.

Fixes a couple of typos. No code changes.

For anyone wondering about the UTC change see CLJ-928

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?

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.

Agree it is good to fix the inconsistency, but I think an infinite sequence of zeros is the correct result, as implied by the current docstring definition.

It's also mathematically cleanest: range should produce an arithmetic progression until the end value is equalled or exceeded.

Empty lists only seem to make sense as a return value when start >= end.

Hi Mike,

Could you explain how you think the docstring definition implies this behavior? Maybe I'm missing something, but I was surprised. For instance, in the case of (range 3 9 0), if start were truly inclusive as the docstring says, the result would be (3), not ().

You mentioned that you think the infinite sequence of 0's is consistent and in keeping with the definition of range. I'm not sure I agree. (0,0] is an empty set of length zero, and [0,0) is an empty set of length zero.

You state that empty list only makes sense for start >= end, except this is not the current behavior. Could you help me understand what you believe the appropriate behavior would be in each of the following three cases? (range 0 10 0), (range 10 0 0), and (range 0 0 0)?

A few options to consider:
1) Fix the docstring to reflect the actual behavior of range.
2) Handle the case of (range 9 3 0) => (9 9 9 ...) to make it consistent with the behavior of (range 3 9 0) => ()
3) Stop allowing a step of zero altogether.

Editing to Add (Note: I don't think the way someone else did something is always the right way, just wanted to do some digging on how other people have handled this in the past):
http://docs.python.org/library/functions.html#range (0 step returns ValueError)
http://www2.tcl.tk/10797 (range returns empty list for a zero step)
http://www.scala-lang.org/api/2.7.7/scala/Range.html (zero step is not allowed)

It does make sense NOT to allow a step of zero (at least to me)... I wasn't going to say anything about this but if other popular languages do not allow 0, then I guess it makes more sense than I originally gave myself credit for... However, if we want to be mathematically correct then the answer would be to return an infinte seq with the start value like below. After all, what is a step of 0? does it make any difference how many steps you take if with every step you cover 0 distance? obviously not...you start from x and you stay at x forever...we do have repeat for this sort of thing though...

(take 5 (range 3 9 0)) => (3 3 3 3 3)

+1 for not allowing 0 step

@Devin quite simple: a lazy sequence of nums starting from x with step 0.0 until it reaches an end value of y (where y > x) is an infinite sequence of x.

Consider the case where start is 0 and end is infinity (the default), you would expect sequences to go as follows:

step +2 : 0 2 4 6 8....
step +1 : 0 1 2 3 4....
step +0 : 0 0 0 0 0....

It would be inconsistent to make 0 a special case, all of these are valid arithmetic progressions. And all of these are consistent with the current docstring.

If you make 0 a special case, then people will need to write special case code to handle it. Consider the code to create a multiplication table for example:

(for [x (range 10)]
(take 10 (range 0 Long/MAX_VALUE x)))

This works fine if you produce an infinite sequence of zeros for step 0, but fails if you create an empty list as a special case for step 0.

As a related issue, I'd personally also favour negative step sizes also producing an infinite sequence. If we don't want to allow this though, then at least the docstring should be changed to say "a lazy seq of non-decreasing nums" and a negative step should throw an error.

Carrying over a message from the clojure-dev list by Stuart Sierra:

• I called the ticket a defect. Does that seem reasonable?

yes

• Does anyone actually use the 0 step behavior in their programs?

not if they have any sense

• Has anyone been bitten by this in the past?

not me

• Is this behavior intentional for some historical reason I don't know about or understand?

I doubt it.

• Has this been brought up before? I couldn't find any reference to it via Google.

Not that I know of.

• Are there performance implications to my patch?

I doubt it. Lazy seqs are not ideal for high-performance code anyway.

• Am I addressing a symptom rather than the problem?

I think the problem is that the result of `range` with a step of 0 was never specified. Don't assume that the tests are specifications. Many of the tests in Clojure were written by over-eager volunteers who defined the tests based on whatever the behavior happened to be. The only specification from the language designer is the docstring. By my reading, that means that `range` with a step of 0 should return an infinite seq of the start value, unless the start and end values are equal.

-S

Carrying over a message by Michal Marczyk:

With a negative step, the comparison is reversed, so that e.g.

(range 3 0 -1)

evaluates to

(3 2 1)

I think this is the useful behaviour; the docstring should perhaps be
adjusted to match it.

Agreed on zero step.

Cheers,
Michał

Adding a new patch after hearing comments. This patch makes (range 9 3 0) => (9 9 9 9 ...), (range 3 9 0) => (3 3 3 3 ...), and () will be returned when (= start end). Also updated the docstring.

Marking as vetted

Patch applies cleanly. We've discussed this issue to death (for as simple as it is). I think it's time to mark it as screened.

For some reason I'm not allowed to edit the attachments list. When you apply the patch, please apply inconsistent_range_fix.diff as that is the most recent version of the fix.

As someone who has to read these tickets, I'd really appreciate it if you could keep the description up to date and accurate, with examples of the current and intended behavior and the strategy to be used in fixing. I can't follow every thread to see what the latest thinking is, especially on a patch like this where the original mission was changed.

Thanks

@Tim: I've removed the other attachments.

@Rich: Understood. I will update the description this evening.

Timothy, I tried applying both of these Sep 26, 2012 patches to latest Clojure master as of that date. I had to apply 0001-make-PersistentQueue-ctor-public.patch by hand since it failed to apply using git or patch. It built fine, but failed to pass several of the Clojure tests. Have you looked into those test failures to see if you can find the cause and fix them? I tested on Ubuntu 11.10 with Oracle JDK 1.6 and 1.7, and saw similar failures with both.

Fixed the patch. Tests pass, created the patch, applied it to a different copy of the source and the tests still pass. So this new patch should be good to go.

Timothy, I'm not sure how you are getting successful results when applying this patch. Can you try the steps below and see what happens for you? I get errors trying to apply the patch with latest Clojure master as of Oct 26, 2012. Also please use the steps on the JIRA workflow page to create a git format patch (http://dev.clojure.org/display/design/JIRA+workflow under "Development" heading).

% git clone git://github.com/clojure/clojure.git
% cd clojure
% patch -p1 < queues.patch
patching file src/clj/clojure/core.clj
patching file src/jvm/clojure/lang/PersistentQueue.java
Hunk #1 FAILED at 32.
1 out of 1 hunk FAILED – saving rejects to file src/jvm/clojure/lang/PersistentQueue.java.rej
patching file test/clojure/test_clojure/data_structures.clj
Hunk #1 succeeded at 123 with fuzz 2.
Hunk #2 succeeded at 861 with fuzz 2.
Hunk #3 FAILED at 872.
1 out of 3 hunks FAILED – saving rejects to file test/clojure/test_clojure/data_structures.clj.rej
patching file test/clojure/test_clojure/java_interop.clj

I was using git apply. I tried the method you show above, and now I'm seeing the same issues you show above.

Just so you know, the preferred way to create and apply patches are the "git format-patch master --stdout > patch.txt" to create a patch (after doing the branching commands described on the JIRA workflow page to create a branch for your changes), and the "git am --keep-cr -s < patch.txt" to apply a patch. If a patch was created that way and applies cleanly with that command, then you are definitely good to go.

The "patch -p1 < patch.txt" command is just a secondary method sometimes used to try to apply patches that aren't in the format produced above, or have errors when applying using that method.

Just so you know, the preferred way to create and apply patches are the "git format-patch master --stdout > patch.txt" to create a patch (after doing the branching commands described on the JIRA workflow page to create a branch for your changes), and the "git am --keep-cr -s < patch.txt" to apply a patch. If a patch was created that way and applies cleanly with that command, then you are definitely good to go.

The "patch -p1 < patch.txt" command is just a secondary method sometimes used to try to apply patches that aren't in the format produced above, or have errors when applying using that method.

added patch

That one applies cleanly and passes all tests. It should show up on the next list of prescreened patches. Thanks.

we don't use the queue* convention elsewhere, e.g. vec and vector. I think queue should take a collection like vec and set. (queue [1 2 3]) could be made to 'adopt' the collection as front.

Patch queue.patch dated Oct 26 2012 no longer applies cleanly after recent CLJ-1000 commits, but only because of one line of changed patch context. It still applies cleanly with "patch -p1 < queue.patch". Not bothering to update the stale patch given Rich's comments suggesting more substantive changes.

See also CLJ-976 (tagged literal support for PersistentQueue)

Don't want to step on Timothy B's toes here, but it looks straightforward to adopt his patch to implement Rich's suggestion. I'd offer to give it a whack if nobody else wants the ticket now.

Related clojure-dev message: https://groups.google.com/forum/?fromgroups#!topic/clojure-dev/fLJWh9A3OuA

and enhancement proposal wiki page: http://dev.clojure.org/display/design/Representing+EDN

Is this fixed now? Tom Faulhaber has regenerated the docs after the recent Clojure 1.5 release, and I think updated other things besides, so it might be.

Nope, not fixed.

This one either slipped by me or came in right when I was changing jobs so didn't stick in my brain.

I'll take a look now. Thanks for the report, Bogdan, and thanks for the bump, Andy to get it on my radar.

Tom, I'm happy to help if you need it. Could you document on a wiki page how autodoc is run here? I couldn't find such a page.

This is fixed with gh-pages commit 919143e (autodoc doesn't follow the regular Clojure release path since it's a website built off the source checkins).

Gabriel, Thanks for the offer. I fixed this one, but may take you up on it if more come up.

There is currently no wiki page about the autodoc process but it's an excellent suggestion. I'll put it on my list to write something up. In the meantime source on the autodoc program itself is at https://github.com/tomfaulhaber/autodoc and a description of how it works is at http://tomfaulhaber.github.io/autodoc. Two caveats: (1) autodoc is currently undergoing a bunch of work (thus this bug fix) in preparation for a new release and (2) the documentation doesn't talk much about how it's used for documenting Clojure itself.

Note that this works as you might have hoped:

(-> [1 2 3] ((fn [args] (apply + args))))

because it expands into:

((fn [args] (apply + args)) [1 2 3])

Your suggestion that -> check for (fn ...) before treating it as a sequence and injecting the argument leaves open the question: Why only (fn ...) should be treated specially? Why not (let ...), (for ...), (doseq ...), etc? And if you go that far, how do you decide what should be allowed and what not?

I agree with Andy, that it's not realistic suggestion to check for fn,let,etc. Perhaps a doc fix would help here but I'm not sure if we just want to call out (fn ...). I'd recommend closing this unless Stephen speaks up.

I'm happy with Andy's synopsis of the problem, and it's reasonable not to change the behaviour of the threading macro specifically for (fn..).

However, this is a mistake that I'm sure many others make/have made and it's hard to diagnose what is going wrong without dumping interpreted form – hardly a reasonable expectation for a novice user.

Before closing this issue, I'd like to see improved failure reporting, such as causing the threading macro to throw a compile error or warning if passed a raw (unwrapped) function declaration (are there legitimate use cases this would affect?).

in my sample project, i used a nested class, but i didnt have to (as pointed by Marko Topolnik). changing the java code to:

abstract class AbstractParent{
final public int x() {return 6;}
}

public class test extends AbstractParent {}

and the clojure to:

(ns call-test.core (:gen-class))
(defn -main [& args](.x ^AbstractParent (test.)))

would produce the same error,

java.lang.IllegalArgumentException: Can't call public method of non-public class: public final int AbstractParent.x()
at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:88)

This issue affects the upcoming netty-4.0 release in which the public modifier of AbstractBootstrap was removed.

To get Netty 4 working with Clojure I had to create a set of public static Java methods for the various inaccessible Netty calls, which I then call from Clojure. A PITA, but works fine. Happy to post code if anyone would find it useful.

Matthew, i kinda left that project after running to these and other troubles (focused on previous Netty until version 4 will become ready and be properly documented), but i'd still like to see your code. you have a github account or a gist with it?

Clojure devs - are there any plans of checking this problem out? it came up from Netty, but the problem is pretty generic

Shlomi: here's a gist with the code I'm using in it. It's not comprehensive, just the bits I needed.

https://gist.github.com/scramjet/5606195