Mac OS, Java 6

The with-redefs doc (v1.4.0) says "These temporary changes will be visible in all threads." That sounds non-thread-safe to me.

In general, changes to var root bindings are not thread safe.

As I understand it, with-redefs is mainly used in test suites to mock out vars. It was introduced when vars became static by default and a lot of testsuites had been using binding for mocking. Maybe the docstring should be amended with something along the lines of: When using this you have to ensure that only a single thread is interacting with redef'd vars.

Behavior find as is, doc string change would be fine.

Patch clj-1104-doc-unsafety-of-concurrent-with-redefs-v1.txt dated Nov 25 2012 updates doc string of with-redefs to make it clear that concurrent use is unsafe.

Rich: do you want a patch for this?

Only if it important to someone, solves some problem.

Please add a patch formatted by "git format-patch" so that attribution is included.

I added the formatted patch a few days ago. Does 'no news is good news' apply here?

And, silly question, will it make it into 1.3? I can't figure out how to tell Jira to show me that.

I fail to see the need for an IAtom, if you want something atom like for couchdb the interfaces are already there. Maybe I ICompareAndSwap. Atoms and couchdb are different so making them appear the same is a bad idea.

http://en.wikipedia.org/wiki/Fallacies_of_Distributed_Computing

http://clojure.org/state one of the distinctions between agents and actors raised in the section titled "Message Passing and Actors" is local vs. distributed and the same distinction can be made between couchdb (regardless of compare and swap) and atoms

This ticket has already been moved into approved backlog. It will be revisited again after the 1.3 release where we will take a closer look at things. For now, this will remain as is.

I was not sure whether the reported arity should match the actual Java method arity or Clojure protocol function arity (including the target). I did the former.

I think it should be the latter. The message is emitted when the protocol methods are being invoked through the corresponding function, so it should be consistent with the errors emitted by regular functions.

+1 for some tests, too. There certainly are tests for reflection warnings and such.

FWIW, I'm happy to take this on if Alex is otherwise occupied.

This seems useful. Rich, would you accept a patch?

Nevermind, just saw that Rich already suggested this on the dev list. Patch away.

Attached is a patch that externalizes and moves the ASM lib to the latest version (4.1), which has support for invokeDynamic if a brave soul wants to emit that instruction. Heed fogus's caveats [3].

ASM in this patch is not re-rooted, but an external Maven dependency. It does have the disadvantage of possibly conflicting with other dependents of ASM, but this is the same approach is used by other JVM langs.

Can classloaders mitigate that problem?

[1] Recent post on clojure-dev, oblivious of [2] https://groups.google.com/d/msg/clojure-dev/iX-ZFPk_zyE/Es3mDOdG3bYJ
[2] https://groups.google.com/d/msg/clojure-dev/ahJXBT1vG68/_fEEM6Lc7LcJ
[3] http://blog.fogus.me/2011/10/14/why-clojure-doesnt-need-invokedynamic-but-it-might-be-nice/

Ran some of Andy Fingerhut's expression microbenchmarks on -master with the patch. There is no difference between performance compared to 1.5.1.

Ghadi, out of curiosity, if you do AOT compilation of some Clojure class files, does it produce byte-for-byte identical .class files after your changes vs. before your changes? If so, that would be pretty high assurance of no problems. If not, it would be good to understand what changed.

the same is also true for TransientVectors

{{(contains? (transient [1 2 3]) 0)}}

false

As expected, TransientSets have the same issue; plus an additional, probably related one.

(:x (transient #{:x}))

nil

(get (transient #{:x}) :x)

nil

This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Rich: Patch doesn't currently apply, but I would like to get your take on approach here. In particular:

1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?
2. what are good names for the interfaces introduced here?

Rebased the patch off the latest pull of master as of 3/25/2011, it should apply cleanly now.

Latest patch does not apply as of f5bcf647

clj-700-patch2.txt does patch cleanly to latest Clojure head as of a few mins ago. No changes to patch except in context around changed lines.

Sigh. Git patches applied via 'git am' are fragile beasts indeed. Look at them the wrong way and they fail to apply.

clj-700-patch3.txt applies cleanly to latest master as of Mar 7, 2012, but not if you use this command:

git am -s < clj-700-patch3.txt

I am pretty sure this is because of DOS CR/LF line endings in the file src/jvm/clojure/lang/Associative.java. The patch does apply cleanly if you use this command:

git am --keep-cr -s < clj-700-patch3.txt

This ticket was changed to Incomplete and waiting on Rich when Stuart Halloway asked for feedback on the approach on 28/Jan/2011. Stuart Sierra changed it to not waiting on Rich on 17/Feb/2012 when he noted the patch didn't apply cleanly. Latest patch clj-700-patch3.txt does apply cleanly, but doesn't change the approach used since the time Stuart Halloway's concern was raised. Should it be marked as waiting on Rich again? Something else?

Patch 4 incorporates patch 3, and brings it up to date on hashing (i.e. uses hasheq).

Removed clj-700-patch3.txt in favor of Stuart Halloway's improved clj-700-patch4.txt dated June 8, 2012.

clj-700-patch5.txt dated June 18, 2012 is the same as Stuart Halloway's clj-700-patch4.txt, except for context lines that have changed in Clojure master since Stuart's patch was created. clj-700-patch4.txt no longer applies cleanly.

Adding clj-700-patch6.txt, which is identical to Stuart Halloway's clj-700-patch4.txt, except that it applies cleanly to latest master as of Aug 19, 2012. Note that as described above, you must use the --keep-cr option to 'git am' when applying this patch for it to succeed. Removing clj-700-patch5.txt, since it no longer applies cleanly.

Patch fails as of commit 1c8eb16a14ce5daefef1df68d2f6b1f143003140

Which patch did you try, and what command did you use? I tried applying clj-700-patch6.txt to the same commit, using the following command, and it applied, albeit with the warning messages shown:

% git am --keep-cr -s < clj-700-patch6.txt
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/jafinger/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq

Note the --keep-cr option, which is necessary for this patch to succeed. It is recommended in the "Screening Tickets" section of the JIRA workflow wiki page here: http://dev.clojure.org/display/design/JIRA+workflow

Presumptuously changing Approval from Incomplete back to None, since the latest patch does apply cleanly if the --keep-cr option is used. It was in Screened state recently, but I'm not so presumptuous as to change it to Screened

Questions posed on the clojure-dev list around how this impacts bit-shift-left: http://groups.google.com/group/clojure-dev/browse_thread/thread/2191cbf0048d8ca6

Patch on CLJ-445 fixes this as well.

This patch fails a test around bit-shifting a BigInt: `(bit-shift-left 1N 10000)`. The reason is that the patch changes the dispatch of (BigInt, Long) from (Object, Object) to (long, int).

Clearly this can't be applied (unless another change makes it possible), but I'm putting it up as a start of the conversation.

My comment from the mailing list:

If the test breaks it likely means Numbers.shiftLeft(long,int) was
selected over Numbers.shiftLeft(Object,Object). Given that 1N is an
Object (one that can exceed the size of a long), the method selection
is incorrect, thus the patch is broken.

The suggestion of "simply" modifying paramArgTypeMatch is not sufficient since the mechanism for preferring one method over another lives in Compiler, and isn't smart enough to make these sorts of decisions.

Considering moving this out of Release.next - soliciting comments from Chas.

I'm afraid I don't have any particular insight into the issues involved at this point. I ran into the problem originally noted a while back, and opened the ticket at Rich's suggestion. I'm sorry if the text of the ticket led anyone down unfruitful paths…

The issues relating to bitshift are moot since the decision was made that bit-shifts are only for 32/64 bit values.

Still a valid issue, but de-prioritized as per Rich.

Modified version of original patch

Alex, would you mind attaching it with a unique file name? I know that JIRA lets us create multiple attachments with the same file name, and I know we can tell them apart by date and the account of the person who uploaded the attachment, but giving them the same name only seems to invite confusion.

Renamed updated patch to unique name

Converted from http://www.assembla.com/spaces/clojure/tickets/457

Stu and Rich talked about making this an error, but it would break some existing code to do so.

Is there a specific question on this?

Stu, you and I went over this but I can't remember exactly what the question was here.

Tentative patch attached.
Have you an example of existing code which is broken by such a patch (as mention by Aaron Bedra)?

The patch intends to do what? We have only a problem description and code. Please enumerate the plan rather than make us decipher the patch.

As a first principle, I don't want Clojure to promise that such recursively defined values are possible.

The proposal here is to catch recursive seq realization (ie when computing the body of a lazy-seq attempts to access the same seq) and throw an exception.

Currently when such a case happens, the recursive access to the seq returns nil. This results in incorrect code seemingly working but producing incorrect results or even incorrect code producing correct results out of luck (see https://groups.google.com/d/topic/clojure/yD941fIxhyE/discussion for such an example).

So this patch moves around the modification to the LazySeq state (f, sv and s fields) before all potentially recursive method call (.sval in the while of .seq and .invoke in .sval) so that, upon reentrance, the state of the LazySeq is coherent and able to convey the fact the seq is already being computed.

Currently a recursive call may find f and sv cleared and concludes the computation is done and the result is in s despite s being unaffected yet.

Currently:

Note that "Being realized" states overlap with Unrealized or Realized.
(NB: "anything" includes null)

With the patch:

That last comment, Christophe, goes a long way to explaining the idea to me, at least. Any chance comments with similar content could be added as part of the patch?

New patch with a comment explaining the expected states.
Note: I tidied the states table up.

// Before calling user code (f.invoke() in sval and, indirectly,
// ((LazySeq)ls).sval() in seq -- and even RT.seq() in seq), ensure that 
// the LazySeq state is in one of these states:
//
// State            f          sv
// ================================
// Unrealized       not null   null
// Realized         null       null
// Being realized   null       this

CLJ-1119 is also fixed by this patch.

Converted from http://www.assembla.com/spaces/clojure/tickets/428

Patch clj-428-change-Sorted-seqFrom-to-take-inclusive-patch-v1.txt dated Apr 28 2013 was written by Mark Engelberg in July 2010, and was attached to a message he sent to the dev thread linked in the description. The approach he takes is described by him in that thread, copied here:

----------------------------------------
Meanwhile, to initiate discussion on how to modify subseq, I've attached a proposed patch. This patch works by modifying the seqFrom method of the Sorted interface to take an additional "inclusive" parameter (i.e., <= and >= are inclusive, < and > are not).

In this patch, I do not address one issue I raised before, which is whether subseq implies by its name that it should return a seq rather than a sequence (in other words nil rather than ()). If seq behavior is desired, it would be necessary to wrap a call to seq around the calls to take-while. But for now, I'm just making the behavior match the current behavior.

Although I think this is the cleanest way to address the extensibility issue with subseq, the change to seqFrom will break anyone who currently is overriding that method. I didn't see any such classes in clojure.contrib, so I don't think it's an issue, but if this is a concern, my other idea is to fix the problem entirely within subseq by changing the calls to next into calls to drop-while. I have coded this to confirm that it works, and can provide that alternative patch if desired.
----------------------------------------

I can also supply a patch that uses drop-while in clojure.core/subseq and rsubseq if such an approach is preferred to the one in this patch.

clj-428-change-Sorted-seqFrom-to-take-inclusive-patch-v2.txt dated Apr 28 2013 is same as clj-428-change-Sorted-seqFrom-to-take-inclusive-patch-v1.txt (soon to be deleted), except it adds tests for subseq and rsubseq, and corrects a bug in that patch. The approach is the same as described above for that patch.

Patch clj-428-change-Sorted-seqFrom-to-take-inclusive-patch-v3.txt dated May 1 2013 is the same as clj-428-change-Sorted-seqFrom-to-take-inclusive-patch-v1.txt, still with the bug fix mentioned for -v2, but with some unnecessary changes removed from the patch. The comments for -v1.txt on the approach still apply.

Converted from http://www.assembla.com/spaces/clojure/tickets/415

alexdmiller said: A simple example I tried for illustration:

user=> (let [a 1 b 2] (assert (= a b)))
#<CompilerException java.lang.AssertionError: Assert failed: (= a b)
 a : 1
 b : 2

fogus said: Of course it's weird if you do something like:

(let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= x y)))
java.lang.AssertionError: Assert failed: (= x y)
 x : 1
 y : 2
 z : 3
 a : 1
 b : 2
 c : 3
 (NO_SOURCE_FILE:0)
</code></pre>

So maybe it could be slightly changed to:
<pre><code>(defmacro assert
  "Evaluates expr and throws an exception if it does not evaluate to logical true."
  {:added "1.0"}
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                          (map (fn [[k# v#]] 
                                                 (when (some #{k#} form#) 
                                                   (str "\t" k# " : " v# sep#))) 
                                               ~bindings)))))))))
</code></pre>

So that. now it's just:
<pre><code>(let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= x y)))
java.lang.AssertionError: Assert failed: (= x y)
 x : 1
 y : 2
 (NO_SOURCE_FILE:0)

:f

fogus said: Hmmm, but that fails entirely for: (let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= [x y] [a c]))). So maybe it's better just to print all of the locals unless you really want to get complicated.
:f

jawolfe said: See also some comments in:

http://groups.google.com/group/clojure-dev/browse_frm/thread/68d49cd7eb4a4899/9afc6be4d3f8ae27?lnk=gst&q=assert#9afc6be4d3f8ae27

Plus one more suggestion to add to the mix: in addition to / instead of printing the locals, how about saving them somewhere. For example, the var assert-bindings could be bound to the map of locals. This way you don't run afoul of infinite/very large sequences, and allow the user to do more detailed interrogation of the bad values (especially useful when some of the locals print opaquely).

stuart.sierra said: Another approach, which I wil willingly donate:
http://github.com/stuartsierra/lazytest/blob/master/src/main/clojure/lazytest/expect.clj

There's one more tweak to fogus's last comment, which I'm actually using. You need to flatten the quoted form before you can use 'some' to check whether the local was used in the form:

(defmacro assert
  "Evaluates expr and throws an exception if it does not evaluate to logical true."
  {:added "1.0"}
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                          (map (fn [[k# v#]] 
                                                 (when (some #{k#} (flatten form#)) 
                                                   (str "\t" k# " : " v# sep#))) 
                                               ~bindings)))))))))

I am holding off on this until we have more solidity around http://dev.clojure.org/display/design/Error+Handling. (Considering, for instance, having all exceptions thrown from Clojure provide access to locals.)

When my pipe dream fades I will come back and screen this before the next release.

Why try to guess what someone wants to do with the locals (or any other context, for that matter) when you can specify a callback (see below). This would have been useful last week when I had an assertion that failed only on the CI box, where no debugger is available.

Rich, at the risk of beating a dead horse, I still think this is a good idea. Debuggers are not always available, and this is an example of where a Lisp is intrinsically capable of providing better information than can be had in other environments. If you want a patch for the code below please mark waiting on me, otherwise please decline this ticket so I stop looking at it.

(def ^:dynamic *assert-handler* nil)

(defn ^{:private true} local-bindings
  "Produces a map of the names of local bindings to their values."
  [env]
  (let [symbols (map key env)]
    (zipmap (map (fn [sym] `(quote ~sym)) symbols) symbols)))

(defmacro assert
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (if *assert-handler*
             (*assert-handler* form# ~bindings)
             (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                            (map (fn [[k# v#]] 
                                                   (when (some #{k#} (flatten form#)) 
                                                     (str "\t" k# " : " v# sep#))) 
                                                 ~bindings))))))))))

A slight improvement I made in my own version of this code: flatten does not affect set literals. So if you do (assert (some #{x} [a b c d])) the value of x will not be printed. Here's a modified flatten that does the job:

(defn symbols [sexp]
  "Returns just the symbols from the expression, including those
   inside literals (sets, maps, lists, vectors)."
  (distinct (filter symbol? (tree-seq coll? seq sexp))))

Attaching git format patch clj-415-assert-prints-locals-v1.txt of Stuart Halloway's version of this idea. I'm not advocating it over the other variations, just getting a file attached to the JIRA ticket.

Converted from http://www.assembla.com/spaces/clojure/tickets/366

stu said: Updating tickets (#370, #366, #374)

Design page is at http://dev.clojure.org/display/design/CLJ+Launcher and should be the basis for all future discussion

Converted from http://www.assembla.com/spaces/clojure/tickets/348
Attachments:
0001-Add-a-test-for-348-reify-shouldn-t-accept-qualified-.patch - https://www.assembla.com/spaces/clojure/documents/d2xUJIxTyr36fseJe5cbLA/download/d2xUJIxTyr36fseJe5cbLA

technomancy said: [file:d2xUJIxTyr36fseJe5cbLA]: A test to expose the unwanted behaviour

richhickey said: I'm not sure the bug is what you say it is, or the resolution should be what you suggest. The true problem is the resolution of key when qualified. Another possibility is to ignore the qualifier there.

technomancy said: Interesting. So it's not appropriate to require auto-gensym here? Why are the rules different for reify methods vs proxy methods?

> (defmacro lookup []
`(proxy [clojure.lang.ILookup] []
(valAt [key] key)))
> (lookup)

Can't use qualified name as parameter: clojure.core/key
[Thrown class java.lang.Exception]

Converted from http://www.assembla.com/spaces/clojure/tickets/326
Attachments:
add-as-of-to-refer.patch - https://www.assembla.com/spaces/clojure/documents/a8SumUvcOr37SmeJe5cbLA/download/a8SumUvcOr37SmeJe5cbLA

cgrand said: [file:a8SumUvcOr37SmeJe5cbLA]: requires application of #325

richhickey said: Do we still need this?

Converted from http://www.assembla.com/spaces/clojure/tickets/291
Attachments:
fixbug291.diff - https://www.assembla.com/spaces/clojure/documents/dfNhoS2Cir3543eJe5cbLA/download/dfNhoS2Cir3543eJe5cbLA

bhurt said: Before this bug gets marked as invalid as well, let me point out that the problem here is that (take-nth 0 any-list) is a meaningless construction- the only question is what to do when this happens. IMHO, the correct behavior is to throw an exception.

ataggart said: [file:dfNhoS2Cir3543eJe5cbLA]: throws IllegalArgumentException on negative step size

Does calling (take-nth 0 ...) cause the problem, or only realizing the result?

I'm not seeing a problem. Calling take-nth and even partially consuming the seq it returns works fine for me:

(take 5 (take-nth 0 [1 2 3]))
;=> (1 1 1 1 1)

Note however that it is returning an infinite lazy seq. The example in the issue description seems to include essentially (doall <infinite-lazy-seq>) which does blow the heap:

(doall (range))

This issue still strikes me as invalid.

(take-nth 0 ...) returns an infinite sequence of the first item:

(take 12 (take-nth 0 [1 2 3]))
=> (1 1 1 1 1 1 1 1 1 1 1 1)

Is something other than this happening on Solaris?

Converted from http://www.assembla.com/spaces/clojure/tickets/270
Attachments:
dont-copy-val-metadata-onto-new-var-value-in-defn.patch - https://www.assembla.com/spaces/clojure/documents/dwK4yssayr37y_eJe5d-aX/download/dwK4yssayr37y_eJe5d-aX
0001-set-meta-on-vars-before-evaluating-their-init-see-27.patch - https://www.assembla.com/spaces/clojure/documents/arrhbiAI4r35lQeJe5cbLr/download/arrhbiAI4r35lQeJe5cbLr

stu said: [file:dwK4yssayr37y_eJe5d-aX]

stu said: The problem happens with defn, but not with def+fn, so I think the original diagnosis is incorrect.

Another stab at diagnosis: The defn macro copies metadata from a var onto its new value, so if you defn a var that already exists, the old var metadata becomes metadata on the new value. I don't know why this would be the right thing to do, and if you eliminate this behavior (see patch) all the Clojure and Contrib tests still pass.

If this is correct, please assign back to me and I will write tests. If this is wrong, please tell me what's going on here so I know how to write the tests.

cgrand said: Child association with ticket #363 was added

cgrand said: I think the original diagnosis is correct. Note that the behavior differ when def iscompiled:

user=> (def #^{:foo "bar"} x 5)
#'user/x
user=> (let [] (defn x [] 5))
#'user/x
user=> (meta x)
{:ns #<Namespace user>, :name x, :file "NO_SOURCE_PATH", :line 83, :arglists ([])}
user=> (meta #'x)
{:ns #<Namespace user>, :name x, :file "NO_SOURCE_PATH", :line 83, :arglists ([])}

See also http://groups.google.com/group/clojure/browse_thread/thread/6afd81896ca368b2#

cgrand said: [file:arrhbiAI4r35lQeJe5cbLr]

cgrand said: My patch aligns DefExpr.eval with DefExpr.emit (first set meta then eval the init value).

cgrand said: Forget it, my current patch is broken.

cgrand said: My current patch is broken because of #352, what is the rational for #352?

Converted from http://www.assembla.com/spaces/clojure/tickets/259

richhickey said: How about some sample case that demonstrates the problem?

hiredman said: Related association with ticket #126 was added

Converted from http://www.assembla.com/spaces/clojure/tickets/252

Converted from http://www.assembla.com/spaces/clojure/tickets/250
Attachments:
add-clojure-debug-flag.patch - https://www.assembla.com/spaces/clojure/documents/aUWn50c64r35E-eJe5aVNr/download/aUWn50c64r35E-eJe5aVNr
assert-report-locals.patch - https://www.assembla.com/spaces/clojure/documents/aUWqLSc64r35E-eJe5aVNr/download/aUWqLSc64r35E-eJe5aVNr

Ignore the old patches. Considering the following implementation, please review and then move ticket to waiting on Stu:

1. RT will check system property "clojure.debug", default to false
2. property will set the root binding for the current *assert*, plus a new *debug* flag. (Debug builds can and will drive other things than just asserts.)
3. does Compile.java need to push *assert* or *debug* as thread local bindings, or can they be root-bound only when compiling clojure?
4. will add *debug* binding to clojure.main/with-bindings. Anywhere else?
5. build.xml should not have to change – system properties will flow through (and build.xml may not be around much longer anyway)
6. once we agree on the approach, I will ping maven plugin and lein owners so that they flow the setting through
7. better assertion messages will be a separate ticket
8. what is the interaction between *debug* and *unchecked-math*? Change checks to (and *unchecked-math* (not *debug*))}?

#3 - root bound only
#4 - should not be in with-bindings for same reason as #3 - we don't want people to set! *debug* nor *assert*
#8 - yes, wrapping that in a helper fn

#6 - my biggest reservation is that this isn't yet informed by maven best practices

System properties can be passed through Maven, so I do not anticipate this being a problem.

However, I would prefer *assert* to remain true by default.

SS is correct about this approach not posing any issue for Maven. In addition, the build could easily be set up to always emit two jars, one "normal", one "debug".

I'd suggest that, while clojure.debug might have broad effect, additional properties should be available to provide fine-grained control over each of the additional "debug"-related parameterizations that might become available in the future.

I'd like to raise a couple of potentially tangential concerns (after now thinking about assertions for a bit in the above context), some or all of which may simply be a result of my lack of understanding in various areas.

Looking at where assert is used in core.clj (only two places AFAICT: validating arguments to derive and checking pre- and post-conditions in fn), it would seem unwise to make it false by default. i.e. non-Named values would be able to get into hierarchies, and pre- and post-conditions would simply be ignored.

It's my understanding that assertions (talking here of the JVM construct, from which Clojure reuses AssertionError) should not be used to validate arguments to public API functions, or used to validate any aspect of a function's normal operation (i.e. "where not to use assertions"). That would imply that derive should throw IllegalArugmentException when necessary, and fn pre- and post-conditions should perhaps throw IllegalStateException – or, in any case, something other than AssertionError via assert. This would match up far better with most functions in core using assert-args rather than assert, the former throwing IllegalArgumentException rather than AssertionError.

That leads me to the question: is assert (and *assert*) intended to be a Clojure construct, or a quasi-interop form?

If the former, then it can roughly have whatever semantics we want, but then it seems like it should not be throwing AssertionError.

If the latter, then AssertionError is appropriate on the JVM, but then we need to take care that assertions can be enabled and disabled at runtime (without having to switch around different builds of Clojure), ideally using the host-defined switches (e.g. -ea and friends) and likely not anything like *assert*. I don't know if this is possible or practical at this point (I presume this would require nontrivial compiler changes).

Hopefully the above is not water under the bridge at this point. Thank you in advance for your forbearance.

Thanks for the useful input Chas. Nothing is concluded yet. I think we should step back and look at the objective here, before moving forward with a solution. Being a dynamic language, there are many things we might want to validate about our programs, where the cost of checking is something we are unwilling to pay in production.

Being a macro, assert has the nice property that, should *assert* not be true during compilation, it generates nil, no conditional test at all. Thus right now it is more like static conditional compilation.

Java assert does have runtime toggle-ability, via -ea as you say. I haven't looked at the bytecode generated for Java asserts, but it might be possible for Clojure assert to do something similar, if the runtime overhead is negligible. It is quite likely that HotSpot has special code for eliding the assertion bytecode given a single check of some flag. I'm just not sure that flag is Class.desiredAssertionStatus.

Whether this turns into changes in assert or pre/post conditions, best practices etc is orthogonal and derived. Currently we don't have a facility to run without the checks. We need to choose between making them disappear during compilation (debug build) or runtime (track -ea) or both. Then we can look at how that is reflected in assert/pre-post and re-examine existing use of both. The "where not to use assertions" doc deals with them categorically, but in not considering their cost, seems unrealistic IMO.

I'd appreciate it if someone could look into how assert is generated and optimized by Java itself.

Bytecode issues continue to be above my pay grade, unfortunately…

A few additional thoughts in response that you may or may not be juggling already:

assert being a macro makes total sense for what it's doing. Trouble is, "compile-time" is a tricky concept in Clojure: there's code-loading-time, AOT-compile-time, and transitively-AOT-compile-time. Given that, it's entirely possible for an application deployed to a production environment that contains a patchwork of code or no code produced by assert usages in various libraries and namespaces depending upon when those libs and ns' were loaded, AOT-compiled, or their dependents AOT-compiled, and the value of *assert* at each of those times. Of course, this is the case for all such macros whose results are dependent upon context-dependent state (I think this was a big issue with clojure.contrib.logging, making it only usable with log4j for a while).

What's really attractive about the JVM assertion mechanism is that it can be parameterized for a given runtime on a per-package basis, if desired. Reimplementing that concept so that assert can be *ns*-sensitive seems like it'd be straightforward, but the compile-time complexity already mentioned remains, and the idea of having two independently-controlled assertion facilities doesn't sound fun.

I know nearly nothing about the CLR, but it would appear that it doesn't provide for anything like runtime-controllable assertions.

The best (dated) evidence I could find says that the compiler sets a special class static final field $assertionsDisabled based on the return of desiredAssertionStatus. HotSpot doesn't do anything special with this, dead code elimination simply makes it go away. The code indeed compiles this way:

11: getstatic #6; //Field $assertionsDisabled:Z
14: ifne 33
17: lload_1
18: lconst_0
19: lcmp
20: ifeq 33
23: new #7; //class java/lang/AssertionError
26: dup
27: ldc #8; //String X should be zero
29: invokespecial #9; //Method java/lang/AssertionError."<init>":(Ljava/lang/Object;)V
32: athrow

Even if we were 100% sure that assertion removal was total, I would still vote for a separate Clojure-level switch, for the following reasons:

1. I have a real and pressing need to disable some assertions, and I don't need the Java interop at all. Arguably others will be in the same boat.
2. there will be multiple debugging facilities over time, and having a top-level debug switch is convenient for Clojure users.
3. Java dis/enabling via command line flags is still possible as a separate feature. We could add this later as a (small) breaking change to our assert, or have a separate java-assert interop form. I am on the fence about which way to go here.
4. I believe it is perfectly fine to throw an AssertionError from a non-Java-assertion-form. We don't believe in a world of a static exception hierarchy, and an assertion in production is a critical failure no matter what you call it. Even Scala does it http://daily-scala.blogspot.com/2010/03/assert-require-assume.html

Rich: awaiting your blessing to move forward on this.

The compiler sets $assertionsDisabled when, in static init code? Is there special support in the classloader for it? Is there a link to the best dated evidence you found?

1. Yes, in static init code
2. There is no special support in the classloader, per Brian Goetz (private correspondence) last week. But dead code elimination is great: "The run-time cost of disabled assertions should indeed be zero for compiled code"

Link: Google "java assert shirazi". (Not posting link because I can't tell in 10 secs whether it includes my session information.)

Is there anything new on this issue? I also look for a convenient way to disable assertions in production.

Converted from http://www.assembla.com/spaces/clojure/tickets/248

awizzard said: Another nice addition could be java.util.NavigableMap, though this is available only since Java 6.

SortedSet-SortedMap-interfaces-248.diff 2012-05-10 is a first pass at the SortedMap interface. If there are no objections, I'll build on the SortedMap implementation to add SortedSet support.

SortedMap-interfaces-248.diff 2012-05-10 supersedes the last diff. It implements and tests the SortedMap interface.

Additionally it includes tests for `sorted-map`, which didn't seem to be tested yet.

SortedMap-SortedSet-interfaces-248.diff 2012-05-18 supersedes the last diff. It implements and tests both SortedMap and SortedSet.

(Concurrent)NavigableMap is not implemented because I think 1.5 is still targeted. In any case, it probably should have its own ticket.

clj-248-SortedMap-SortedSet-interfaces-patch2.txt dated May 24, 2012 is simply an updated version of SortedMap-SortedSet-interfaces-248.diff dated May 17, 2012. The latter patch no longer applied cleanly.

The new patch combines what was internally multiple git commits into one, simply because it was easier for me to create the patch that way. I left out the addition of *.swp and *.swo files to the .gitignore file. Builds and tests cleanly on Oracle/Apple JDK 1.6 on Mac OS X 10.6.8.

I don't know why Priority changed to Blocker when I changed Patch to "Code and Test". I didn't try to do that – suspect that something with the web UI is doing that automatically somehow.

Added CLJ-1008 for NavigableMap interface enhancement.

Converted from http://www.assembla.com/spaces/clojure/tickets/211
Attachments:
destructuring-fns.diff - https://www.assembla.com/spaces/clojure/documents/aHWQ_W06Kr3O89eJe5afGb/download/aHWQ_W06Kr3O89eJe5afGb

chouser@n01se.net said: [file:aHWQ_W06Kr3O89eJe5afGb]: [PATCH] Support -> and ->> in destructuring forms.

cgrand said: I think the current patch suffers from the problem described here http://groups.google.com/group/clojure-dev/msg/80ba7fad2ff04708 too.

richhickey said: so, don't use syntax-quote, just use clojure.core/->

chouser@n01se.net said: Only -> and ->> are actually legal here anyway – if you've locally bound foo to -> there's not really any good reason to think (fn [(foo inc a)] a) should work. And if you've redefined -> or ->> to mean something else in your ns, do we need to catch that at compile time, or is it okay to emit the rearranged code and see what happens?

In short, would '#{> ->> clojure.core/> clojure.core/->>} be sufficient?

cgrand said: Only -> and ->> are legal here but what if they are aliased or shadowed? Instead of testing the symboil per se I would check, if:

• the symbol is not in &env
• the symbol resolve to #'clojure.core/> or #'clojure.core/>>

(when-not (&env (first b)) (#{#'clojure.core/-> #'clojure.core/->>} (resolve (first b))))

but it requires to change destructure's sig to pass the env around

Rich: Are you assigned to this by accident? If so, please deassign yourself.

Converted from http://www.assembla.com/spaces/clojure/tickets/163
Attachments:
fixbug163.diff - https://www.assembla.com/spaces/clojure/documents/bH0XMCFjur3PLMeJe5aVNr/download/bH0XMCFjur3PLMeJe5aVNr

laurentpetit said: [file:bH0XMCFjur3PLMeJe5aVNr]

richhickey said: I don't want to recommend, in = doc, that people should prefer == for any case. People should always prefer =. If there is a perf, difference we can make that go away. Then the only difference with == is that it will fail on non-numbers, and that should be the only reason to choose it.

richhickey said: Updating tickets (#94, #96, #104, #119, #163)

laurentpetit said: Richn, by "will fail on non-numbers", do you mean "should throw an exception" (and thus the patch must change the code), or just as it works today :

(== :a :a)
false

?

richhickey said: I've fixed the code so == on non-numbers throws

Converted from http://www.assembla.com/spaces/clojure/tickets/127

jmcconnell said: I'd be happy to take this up with the GAE folks if it winds up looking like this is something they should probably allow or if we need any further information from them on their policies.

richhickey said: Updating tickets (#127, #128, #129, #130)

mikehinchey said: GAE made some changes a few weeks ago, maybe changed this because I'm able to load from .clj files now (not the servlet, of course, which must be gen-class).

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

mattrevelle said: J. McConnell, was this issue resolved by a change in GAE policy?

Converted from http://www.assembla.com/spaces/clojure/tickets/113

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)

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

seems like the code that is emitted in the static init for namespace classes could be emitted into a init_ns() static method and the static init could call init_ns(). then RT.load could call init_ns() to get the behavior of reloading an AOT compiled namespace.

looking at the compiler it looks like most of what I mentioned above is already implemented, just need RT to reflectively call load() on the namespace class in the right place

Converted from http://www.assembla.com/spaces/clojure/tickets/84
Attachments:
genclass-allow-unresolved-classname.patch - https://www.assembla.com/spaces/clojure/documents/cWS6Aww30r3RbzeJe5afGb/download/cWS6Aww30r3RbzeJe5afGb

oranenj said: [file:cWS6Aww30r3RbzeJe5afGb]: on comment 1

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)

The approach take in the initial patch (delaying resolution of symbols into classes) is fine: gen-class makes no promise about when this happens, and the dynamic approach feels more consistent with Clojure. I think the proposed (but not implemented) use of string/symbol to control when class names are resolved is a bad idea: magical and not implied by the types.

Needed:

• update the patch to apply cleanly
• consider whether totype could live in clojure.reflect.java. (Beware load order dependencies.)

Wow, 18-month-old patch, back to haunt me for Halloway'een

So what does it mean that the assignee is Rich, but it's waiting on me?

I am using Approval = Incomplete plus Waiting On = Someone to let submitters know that there is feedback waiting, and that they can move the ticket forward by acting on it. The distinction is opportunity to contribute (something has been provided to let you move forward) vs. expectation of contribution.

Converted from http://www.assembla.com/spaces/clojure/tickets/69
Attachments:
keyset.patch - https://www.assembla.com/spaces/clojure/documents/dKgE6mw3Gr3O2PeJe5afGb/download/dKgE6mw3Gr3O2PeJe5afGb

oranenj said: [file:dKgE6mw3Gr3O2PeJe5afGb]

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)

patch not in correct format

Converted from http://www.assembla.com/spaces/clojure/tickets/47

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)

aredington said: This appears to still be a problem with the generated bytecode in 1.3.0. Examining the bytecode for last, the problem has moved to invokeStatic:

<pre>
public static java.lang.Object invokeStatic(java.lang.Object) throws java.lang.Exception;
Code:
0: aload_0
1: invokestatic #50; //Method clojure/core$next.invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
4: dup
5: ifnull 25
8: getstatic #56; //Field java/lang/Boolean.FALSE:Ljava/lang/Boolean;
11: if_acmpeq 26
14: aload_0
15: invokestatic #50; //Method clojure/core$next.invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
18: astore_0
19: goto 0
22: goto 30
25: pop
26: aload_0
27: invokestatic #59; //Method clojure/core$first.invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
30: areturn
</pre>

Line number 22 is an unreachable goto given the prior goto on line 19.

Converted from http://www.assembla.com/spaces/clojure/tickets/42

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)

richhickey said: Updating tickets (#42, #71)

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

Converted from http://www.assembla.com/spaces/clojure/tickets/2

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

Patch demonstrates idea, not ready for prime time.

I think the decision of having to specify either a Closeable resource or a close function for an existing non-Closeable resource in with-open is quite awkward, because they have completely different meaning.

  (let [foo (open-my-custom-resource "foo.bar")]
    (with-open [r (reader "foo.txt")
                foo #(.terminate foo)]
      (do-stuff r foo)))

I think a CloseableResource protocol that can be extended to custom types as implemented in the patch to CLJ-308 is somewhat easier to use. Extend it once, and then you can use open-my-custom-resource in with-open just like reader/writer and friends...

That said, Scopes can still be useful, but I'd vote for handling the "how should that resource be closed" question by a protocol. Then the with-open helper can simply add

(swap! *scope* conj (fn [] (clojure.core.protocols/close ~(bindings 0))))

and cleanup-scope only needs to apply each fn without having to distinguish Closeables from fns.