Patch 0001-Add-hasheq-based-fallback-to-Util.compare.patch dated Apr 17 2013 applies cleanly to latest master, but causes several unit tests in data_structures.clj to fail. These are the kinds of things you would expect to fail with the change made in the patch, because the failing tests expect an exception to be thrown when comparing objects that don't implement Comparable, and with this patch's changes they no longer do. If this patch's change is desired, those tests should probably be removed.

Thanks Andy, I can't believe I've forgotten to re-run the tests.

Anyway, I'm attaching a new version of the patch that deletes the few sorted-set and sorted-set-by invocations that check that an exception is thrown when creating sorted sets containing non-Comparables.

New version of the patch.

Tassilo, you say that one of your use cases is sorted collections. Note that any compare function that returns 0 for two values will cause sorted sets and maps to treat the two compared values as equal, and at most one of them will get into the ordered set/map, the second treated as a duplicate, even though the values are not equal. See https://github.com/jafingerhut/thalia/blob/master/doc/other-topics/comparators.md#comparators-for-sorted-sets-and-maps-are-easy-to-get-wrong for an example (not based on your modified compare, but on a comparator that returns 0 for non-equal items).

With your proposed change to compare, this occurrence would become very dependent upon the hash function used. Probably quite rare, but it would crop up from time to time, and could potentially be very difficult to detect and track down the source.

Hi Andy, you are right. It's possible to add an explicit equals()-check in cases the hashcodes are the same, but I think there's nothing you can do in the hashcodes-equal-but-objects-are-not case. That is, there's no way to ensure the rule sgn(compare(x, y)) == -sgn(compare(y, x)), the transitivity rule, and the compare(x, y)==0 ==> sgn(compare(x, z))==sgn(compare(y, z)) for all z rule.

I'm closing that ticket.

Mac OS X Clojure 1.5.1

I think this is a duplicate of http://dev.clojure.org/jira/browse/CLJ-1093

You are right, sorry for the duplication. I searched for empty record in JIRA and somehow missed that ticket.

Close as duplicate of CLJ-1093.

I guess you didn't try to actually use the function to see if it is actually broken...

m is not a argument vector, those are below in the function clauses.

I did, but as it turns out the error was somewhere else and I can't reproduce it within a "clean" environment. Sorry for this hasty ticket.

Dmitry, do you think it is OK to close this ticket as declined, meaning no change will be made in response to it? I can do that for you, if you wish.

Andy, I'll close it myself if you don't mind. I would done it earlier, but didn't have sufficient rights in JIRA.

the attached patch would turn

...
public static Var ENQUEUE = RT.var("greenmail.smtp","enqueue");
...
ENQUEUE.fn().invoke(userManager, state.getMessage());
...

in to something like

...
public static VRef ENQUEUE = API.vref("greenmail.smtp/enqueue");
...
ENQUEUE.fn().invoke(userManager, state.getMessage());
...

what is the value of VRefs over using Vars directly?

using this from a java api, it looks like if the namespace the var is in is not loaded when you go to create a VRef it will return null, generally my java code that calls clojure looks something like

public static Var FOO = RT.var("namespace", "name");
public static NAMESPACE = Symbol.intern("namespace");
public static Var REQUIRE = RT.var("clojure", "require");

static {
  REQUIRE.invoke(NAMESPACE);
}

can you tell me without checking the java/jvm spec if FOO is null? does the static init block run before or after static fields are init'ed? returning null just seems like a bad idea.

Per discussion on the ticket and with Rich, the wrapper-free approach (Apr 3 patch) is preferred.

Hi Kevin,

The purpose of not returning Vars is outlined in the design discussion. That said, it is possible to return IFn, which does not drag in too much implementation detail (just a javadoc config tweak, see CLJ-1190). Today's patch returns IFn, which addresses the wrapper ickiness demonstrated by your code examples.

Java static initializers run in lexical order, and I trust Java programmers to know Java.

I can think of several options other than returning null when a var is not available, and they are all complecting, inconsistent with Clojure, or both.

Hey,

Always returning the var is very consistent with clojure, it is what RT.var does. It is what the var lookups emitted by the compiler do. RT.var is most likely the point through which most java that calls clojure goes at the moment.

As to "hiding" vars, how about creating a clojure.lang.ILink interface with both deref() and fn(), have Var implement it. The previous discussion I see linked all seems to presume hiding Var without discussion of why it should be hidden. What I see Rich saying is:

So RT.var is the right idea. It would be nice to hide the Var class,
but unfortunately we can't make ClojureAPI.var() return both IFn and
IDeref without inventing a common subinterface. There are other
similar details.

and

We will need an interface unifying IDeref and IFn for the return type
of API.var()

It seems like Rich is suggesting that ILink or whatever should also bring in IFn, but I wonder if having a fn() that does the cast for you is an acceptable alternative to that.

The API should be called var, not fn, and should return IFn. Also, I really want the logic of RT.var (i.e. Var.intern) to be used, not this new logic. Please find another way to handle the string/symbol support.

Attached the straightforward patch which solves this issue.

The example shows valid Clojure code.

All

Fixed. Thanks for pointing this out.

Clojure 1.4

CLJ-1202 addresses this exact issue with the same fix and includes tests

Irakli, it is an explicitly documented feature that conj puts new items in different places for lists (at the beginning) and vectors (at the end). rest is explicitly documented to call seq on its argument. See their doc strings.

I don't know if it is explicitly documented, or just long-standing practice, that vectors and seqs with the the same sequence of values in them are equal. I think that a lot of existing code would break if Clojure changed so those were not equal.

Patch clj-1168-patch-v1.txt dated Feb 19 2013 has been tested manually with these results:

% java -cp clojure.jar clojure.main

(ran some simple tests to ensure repl still works, and obeys -Dclojure.read.eval=unknown setting)

% java -Dclojure.read.eval=unknown -jar clojure.jar -e '(println (+ 1 2))'
3

The patch works for me, but I think it would be safer for the patch to treat *read-eval* :unknown as false for the purpose of -e. It's unlikely that anyone wants to -e '#=(boom)'. Also, there's no need for the let to capture the cur-read-eval. I suggest something like this for the patch (updated):

(defn- read-never-unknown [read-fn & args]
      (binding [*read-eval* (true? *read-eval*)]
         (apply read-fn args)))

I have asked on the Leiningen email list whether treating read-eval :unknown as false for the purpose of -e would break Leiningen functionality, and will report back here when I find out.

I don't have an answer from Leiningen folks yet, but I do have another thought related to Steve's comment:

The current behavior is to treat *read-eval* as true for the purposes of reading lines in the REPL, if -Dclojure.read.eval=unknown was specified on the command line. Why should expressions given after -e be any more restricted? Whoever is invoking the JVM has complete control.

If they really want *read-eval* to be false when reading the expressions after -e, just use -Dclojure.read.eval=false instead of =unknown.

You're right, the user has control. I was just thinking that the user might not appreciate the details and it would be safer to default to the false behavior. However, I failed to consider the REPL treatment – that's a good point.

In any case, the logic for the binding in the patch could be simplified. I think this would work for the intended result:

(defn- read-never-unknown [read-fn & args]
      (binding [*read-eval* (and *read-eval* true)]
         (apply read-fn args)))

Sorry, if my comments have descended into bike-shedding.

I implemented "with-read-known" patch after input from Rich. Helpers that establish bindings should take a closure or fn body, rather than carrying around explicit fns and arg lists as the original 19 Feb patch does.

Marking screened as of the Feb 22 with-read-known patch.

Steve: More importantly, any time you call (eval (read ...)), there's no reason for *read-eval* to be anything other than true.

Newer patch works. My verification procedure:

• Patched master (should probably use an RC instead)
• Compiled and installed (mvn install) with version = 1.5.0-clj1168 in pom.xml
• Set up fresh Leiningen project including these options:

  :dependencies [[org.clojure/clojure "1.5.0-clj1168"]]
  :jvm-opts ["-Dclojure.read.eval=unknown"]

• Verify: `lein repl` succeeds
  • Verify: => (read-string "foo") fails with "Reading disallowed - *read-eval* bound to :unknown"
  • Verify: => #=(+ 1 2) prints 3 (read is fully available to REPL)
• Write -main fn in project as

  (defn -main [& args]
    (println *read-eval*)
    (println (read-string (first args))))

• Verify: `lein run foo` prints :unknown and then fails with "Reading disallowed" error.
• Bind *read-eval* to false in -main
• Verify: `lein run foo` prints foo.

=> (last (range 0.0 10000000.0 0.1))
9999999.98112945
=> (last (range' 0.0 10000000.0 0.1))
9999999.9

Range is incremental by design, and that is how floats work. Something with the desired behavior would need to be a different fn with a different name.

"Returns a lazy seq of nums from start (inclusive) to end (exclusive), by step"

What specifically about that wording specifically suggests that the implementation will use naive increment-and-recurse behaviour? My reading is that the function will return a lazy sequence of numbers from start to end separated by step, not separated by 'almost step'.

This implementation leads to unexpected behaviour with bounds:

=> (count (range 0 100 1))
100
=> (count (range 0 10 0.1))
101

http://www.ima.umn.edu/~arnold/455.f96/disasters.html

range could avoid this issue cleanly by converting floats to bigdecimals (let me know if you think this is a good idea?)

I ran into this problem recently, and have to say it was pretty ugly. This is how I avoided the issue:

(defn rangef
"Returns a sequence of n numbers from start to end inclusive."
[start end n]
(for [i (range 0 n)]
(+ start (* i (/ (- end start) (dec n))))))

Hope that helps any disillusioned float users out there, or just pass in BigDecimals to range instead of floats... I would go so far as to say using floats with range as it stands is almost always going to end in tears (or worse as Stephen describes ).

[and just to be clear if it is considered an error, it would be nice if range explicitly forbade it]

It appears from the doc string that this function does exactly what it claims it will do, without any changes.

You're right.
Somehow I parsed the doc string wrongly. My sincere apologies! Can be marked invalid.

Closing as declined, since submitter agrees that code behavior and documentation match.

It is easy for tools that consume Clojure to hide stack traces for those who do not want to see them. If Clojure itself eats stack traces, it is impossible for those of us who do want to see them to get them back.

Please do this kind of work in tool (if at all) and make it optional for users.

It's worth noting that there was a breakin to rubygems.org over the weekend that was caused by what amounts to this same issue, but with YAML: https://news.ycombinator.com/item?id=5141138

The current default is both dangerous and undocumented; this needs to change.

Seconded!

Thirded.

For what it's worth, on the topic of breaking changes I'd much rather my old code break than continue to work with an unnoticed security hole.

Fourthed. I hope folks will pitch in and help deal with any breakage.

New patch attached, {{CLJ-1185.patch}}.

This patch eliminates the use of *print-dup* in the compiler entirely. read-string continues to be used to support values that can only be represented via tagged reader literals; I don't think there's any way around that, since the mapping between particular data readers and values' types are buried in individual print-method implementations.

All tests pass (including cases like (eval (list + 1 2 3)) as discussed in the ML thread referenced in the issue description), and a variety of sanity checks around evaluation and compilation tooling (e.g. AOT, nREPL, introspection utilities in Leiningen/Reply/Counterclockwise) all work as expected.

Of course, if this is applied, then any usage of read over trusted content containing #= forms will need to be done with *read-eval* bound to true.

To aid in testing, a Clojure build (1.5.0-RC4) + this patch is available here:

[com.cemerick/clojure "1.5.0-SNAPSHOT"]

Note that you'll have to exclude the standard Clojure dependency from your project in order for this one to take precedence; you can do this by adding

:exclusions [org.clojure/clojure]

to your project.clj.

I ran the Sonian test base against this patch. I wouldn't say our test cases are comprehensive, but they're not trivial. We talk to databases through JDBC, and we deal with lots of different data types (BigInts, Strings, Dates, and such).

I had to make a few small changes to our code base, because we rely pretty heavily on print-dup to serialize non-user originated data. We have to add corresponding binding blocks when reading to set read-eval to true. Other than that, the tests all passed.

+1

Thanks to all that have tested the patch! Looks like we've made some progress. I'm going to post to the main Clojure ML now and hopefully get more yay/nay votes.

clj-1153-patch-v2.txt dated Feb 2 2013 is functionally identical to Chas Emerick's CLJ-1185.patch dated Feb 1 2013, and no retesting is needed by anyone because of it.

The only change I have made to his patch is: the doc string for read-eval now says its default value is false.

Also removed my original didn't-fix-enough-things patch from Jan 30 2013.

I'm bumping the Priority field from its default value to "Major" to reflect recent events. (I personally feel "blocker" would be more appropriate, or at least "critical", but I don't want to step on any toes.)

https://github.com/clojure/clojure/commit/974a64c06917861b7557fd73154254195b2de548

This ticket is an excellent example of a terrible ticket.

1) It does not lead with a problem statement. The title takes the form akin to "I want X". It proposes a tactic.

2) The description is woefully inadequate. Here too, no problem statement. Descriptions should point to any discussions, but discussions are long and rambling, and no substitute for a succinct problem statement in the description. Descriptions need to be maintained, with the current strategy and name of best patch.

3) No resolution strategy. Just patches attached. How is a reviewer supposed to assess your patch if you don't even bother stating what the problem is and what your approach will be in fixing it, how that approach does fix it, and what if any tradeoffs are being made?

4) The change being requested would be a breaking change. That should be made extremely clear in the description, and doubles the threshold for quality of motivation and implementation.

5) Any breaking change would have to carefully enumerate what would break and when, what the migration strategy would be etc.

6) The patch breaks the compiler. If you don't understand how the compiler works, and why features are there, you shouldn't submit patches that alter it. The only assessments made in comments are 'it works for me', which, while useful anecdotes, are insufficient.

While the ticket itself was bad, the uncritical rallying behind it was more troubling. This is not how Clojure was built, and will not be how it is maintained.

In the end, the ticket proposed a tactic, and that tactic has been rejected. Read safety has been addressed by other means.

Is this a duplicate with CLJ-1086, CLJ-1121, or the combination of those two?

If arity 0 is a new aspect of this ticket, what would you expect the arity 0 versions to be transformed into? nil?

Surely a duplicate - but Jira's search couldn't find those! (used "->", "thread-last" etc as search queries).

While fairly unlikely to use / noisy to implement, the thing about feeding 0-arities to the threading macros is that they throw in exchange an unrelated exception rather than e.g. "ArityException Wrong number of args (0) passed to: core$->".

Don't know if this is simply how macros (can) work, but it called my attention.

Feel free to close my issue.

I will go ahead and close this case as a duplicate. FYI, below is the behavior if the CLJ-1121 patch CLJ-1121-v002.patch and the CLJ-1083 patch CLJ-1083-attachments/better-throw-arity-messages.diff are both applied (the second one improves the error message in the 0-arity case):

Clojure 1.5.0-master-SNAPSHOT
user=> (->)
ArityException Wrong number of args (0) passed to: core/-> clojure.lang.Compiler.macroexpand1 (Compiler.java:6472)
user=> (-> 5)
5
user=> (->>)
ArityException Wrong number of args (0) passed to: core/->> clojure.lang.Compiler.macroexpand1 (Compiler.java:6472)
user=> (->> 5)
5

The issue of a poor error message with 0-arity threading macros is improved by the patch attached to ticket CLJ-1083, and adding handling of the 1-arity case is improved by the patch attached to ticket CLJ-1121.

There's been some discussion on clojure-dev around this issue: https://groups.google.com/d/topic/clojure-dev/qdDRNfEVfQ8/discussion

An issue I brought up in the email thread is consistency: vector binding works with anything nthable, map binding works with anything associative. With my current patch (empty-list-destructuring-CLJ-1140-12.30.12.diff), only ISeqs are supported for kwarg map binding.

I'd prefer it work with anything seqable, and can provide a patch that does that. I would go ahead and do so, but now that this ticket is now Approval: OK, I didn't want to alter what had been OK'ed. Let me know if you want a patch that adds support for anything seqable.

• Clojure: 1.5.0-RC1
• Java: Java 1.7.0_05 Java HotSpot(TM) 64-Bit Server VM
• Leiningen: 2.0.0-preview10
• lein-swank: 1.4.4
• lein-ritz: 0.6.0

I suppose I should add that I'm using Mac OS X 10.8.2

Have you noticed that swank-clojure [1] is deprecated, i.e. no longer under active development, and the developer recommends that you use nrepl.el [2] instead?

[1] https://github.com/technomancy/swank-clojure
[2] https://github.com/kingtim/nrepl.el

It also appears that someone has created an update to lein-swank (to version 1.4.5) that works with Clojure 1.5.0-RC1:

https://groups.google.com/forum/#!topic/clojure/CAiajyDWJJg

(in particular Toby Crawley's post on Dec 26 2012)

Yes, this can be closed.

Submitter agrees this issue was resolved by changes in other software outside of Clojure.

Howard, is this perhaps a duplicate of CLJ-939? Let me know, and I can close this ticket as a duplicate if so.

Yes, looks like a dupe to me. Sorry about that, I did do a search for existing issue before adding mine, but they can be hard to find.

Closed as duplicate of CLJ-939.

Windows 7, Maven 3.0.4

Most likely this should be closed as a duplicate of CLJ-1076. The symptoms sound the same, and CLJ-1076 has a patch for it that should fix the problem.

Hi Andy - yes this looks like a duplicate, thanks for spotting. Should be closed.

Duplicate of CLJ-1076

screeners, please make sure this has tests before passing on

Tests added

Patch applies, fixes the bug. All tests pass.

Screened

I see this has already been screened, but FWIW I tested the repl with this patch and the behavior works for me.

reify is not documented to support these, so this should be classified as an enhancement

Vetted.

Tests pass, code looks reasonable. Would appreciate additional review.

Tests also pass here. Looked through the code and played with a patched version of Clojure. I can't see a problem with it.

FYI, gvec.clj has two loops which return primitives and thus was formerly boxed.

Has it been decided whether as-> supports de-structuring or not? Right now it is inconsistent.

(as-> [0 1]
      [a b]
  [(inc a) (inc b)])

 => [1 2]



(as-> {:a 0 :b 1}
      {:keys [a b]}
  {:a (inc a) :b (inc b)})

 => {:keys [1 2]}

Also, what about changing the name to >as? ->as reads like "pipeline as name", whereas as> implies that you are starting a new pipeline.

(-> 0
  (->as name 
    (...))

Makes it clearer that you are consuming a pipeline and the expressions within ->as are no longer being influenced by ->.

Has the addition of ->>as been considered? That way you can use it inside a ->> like this:

(->> a 
  (->>as name
    (...)))

(-> a 
  (->as name
    (...)))

The signature would be [name forms init-expr].

Oracle Java 1.5.0_22, at least

Patch patch-enable-java-5-to-pass-tests-v1.txt dated Nov 19 2012 enables at least Oracle Java 1.5.0_22 to build and pass all tests in latest master.

No, tt doesn't implement a ForkJoin library. It simply declares some Clojure fj functions to throw an exception if they are called. In today's Clojure tests they never are.

Updated patch with proper name and email address.

IMO, more tests are always good. Vetted.

This is the wrong approach to this problem. A better approach is to make reducers work even if no fj support is present by simply defining fold as sequential reduce in that case.

Can you add some unit tests that fail with the current code but succeed with the change?

Revised patch with unit test that fails in master and succeeds with patch

vetting

This came up when we were trying to diff two datasets by putting them in sets and comparing. We had Integers because they came back from JDBC that way.

I'm going to take a look and try to get this shipped. It hit us and I would love to to see it in 1.5

I applied the patch on master (ca465d6d) and it applied cleanly and fixes the issue.

Instead of removing m.hashCode() != hashCode(), perhaps we should use `Util.hasheq()` instead. It already seems to special case numbers (https://github.com/clojure/clojure/blob/f48d024e97/src/jvm/clojure/lang/Util.java#L164-L172) and won't have the potential performance impact that skipping hashCode checks could bring.

Attaching a patch with an alternate fix for this issue that does not skip hashCode checking.

It passes all existing tests and fixes this issue.

I want to benchmark the difference, too.

On further thought and comparison to APersistentMap, I'm not sure if that's the best use of Util.hasheq(). I can't find good reference on the semantic differences and am not familiar enough with the Clojure source to infer it, yet.

Bo, I don't see you listed on the contributors list. Did you send in a CA?

Both of the patches above are not complete. APersistentSet equiv calls equals. APersistentMap has two separate ways of handling this. This is the path that the fix should take.

This patch addresses the difference between equals and equiv.

The Set equality code currently in Clojure uses .hashCode to quickly bail out of comparisons in both .equals and .equiv. This feels wrong since .equals and .equiv presume different hashes. The map equality code avoids the problem by not checking any hash.

Aaron's 2/13 patch makes set equality work like map equality, not checking the hash. (I think a much shorter patch that accomplishes the same thing merely drops the call to hash.)

It seems to me a separate problem that both hash and set are broken for Java .equals rules, because of the equiv-based handling of their keys.

I think this needs more consideration.

Notes from discussion with Rich:

1. Aaron's 2/2/13 patch, which mimics map logic into set, is the preferred approach.

2. The broader issue I was worried about is the semantic collision between Map's containsKey and Associative's containsKey. This is out of scope here, and perhaps ever.

Any chance this will make 1.5?

Attached patch with tests

Another discussion thread: https://groups.google.com/forum/?fromgroups=#!topic/clojure/xPDDybYGvmc

And before creating this patch, I did a diff between Clojure 1.4 and 1.5-beta1 source code, and verified that every other function with :added metadata that has been added since 1.4 says "1.5". Only re-quote-replacement was mislabeled in this way.

As you'd expect, this patch applies cleanly and improves the correctness of the documentation.

changes-draft-v1.md is not a patch, but an outline of a proposed new changes.md file for Clojure 1.5 with some of the content fleshed out. It still has lots of occurrences of "TBD" for "To Be Documented" in it.

It does mention every ticket that had a patch committed since Clojure 1.4.0 until Nov 15 2012.

I am hoping someone who is more knowledgeable of the "TBD" parts takes this and runs with it.

changes-draft-v2.md is very similar to the earlier changes-draft-v1.md described above, but has a few additions.

V3 of the draft...should be almost good to go. Someone with more info on reducers should take a look at that section as have yet to actually use them much.

Removed V2 of the draft as Timothy's V3 had all of it and more.

Fleshed out the reducers section a bit.

Alright, I think this is ready for a final review by someone besides me.

Editorial cleanup

Christopher, isn't this file intended to be in Markdown format, not HTML?

uploading the correct md file

changes-draft-v6.md same as v5, except for a couple of spelling corrections.

Missing desc on when->. Fixed in v7 of the doc

"1.1 Clojure 1.5 requires Java 1.6 or later"

did you mean "building Clojure 1.5"?

I don't know that anything requires 1.6

test->, let-> when->

are now:

cond->, as-> and some->

I'll put up an updated version soon, but that headline wasn't properly updated to match the later occurrence of it in the body, which is: "Clojure 1.5 reducers library requires Java 6 or later". Is it true that the ForkJoin library requires Java 6 or later? If not, how can it be made to work with Java 5?

changes-draft-v8.md updates the table of contents headings to match those in the body, and updates names of new threading macros.

changes-draft-v8.md, section 2.4, needs an update for description of some->. The document says "logical true" where it should say "not nil". Same comment applies to some->>. The point is that false will thread through the forms. This is a change from the replaced when-> (in 1.5-beta1).

updated to reflect changes to some-> and some->>

Sorry to nit pick, but there are two more "logical true" phrases in v9 that need to change to "not nil" in those some-> and some->> descriptions.

Not a problem. Thanks for looking it over!

I find the following sentence a little vague: "Clojure 1.5 can still be compiled and run with Java 5, but the test suite will not pass due to the lack of support for ForkJoin."

The problem is the application of the "but" to both parts of the conjunction. Isn't the intent actually: "Clojure can run with Java version 1.5 or later. Running the Clojure build requires 1.6, in order to test features that work with ForkJoin."

Stuart H, that is almost the intent, and probably close enough for this kind of documentation.

The full gory details are as follows:

Clojure can build and run with Java version 1.5 or later. A Java version 1.5 build of Clojure only works if you do not run the test suite, e.g. by using the command "ant jar". To build Clojure including running the test suite, or to use the new reducers library, requires Java 1.6 or later.

If the patch for CLJ-1109 is applied, the full gory details become simpler to state:

Clojure can build and run with Java version 1.5 or later. Using the new reducers library requires Java 1.6 or later.

changes-draft-v11.md changes the description of Java 5 limitations to match the current gory details, and hopefully address Stuart H's concerns raised above.

Attached patch changes do-copy methods to test for -1

Herwig, can you elaborate on "the specification is pretty clear in that InputStream implementations are allowed to return reads of zero bytes before returning more bytes"?

I ask because in the very InputStream documentation page you link to, it seems to explicitly say the opposite of what you claim: "If no byte is available because the stream is at the end of the file, the value -1 is returned; otherwise, at least one byte is read and stored into b."

I reread the spec for InputStream.read and it clearly says

"If no byte is available because the stream is at the end of the file, the value -1 is returned; otherwise, at least one byte is read and stored into b."

The reason I originally thought of this as an issue was a misbehaving ServletInputStream from Jetty.

I closed the issue as declined. Sorry for the noise!

Patch attached to only refer in the utility vars if in the user namespace.

It would probably be better to test with ns-name (as opposed to comparing strings):

(= 'user (ns-name ns))

I don't follow how it is required to call `clojure.main/repl` for every input

Gah, of course. :-/ Patch updated.

@Kevin: It's not required, but I found it far more straightforward to not try to pretend that the underlying transport was stream-based when it's actually message-based. It also means that sessions can be very lightweight: unless code is being evaluated within a session, it is not occupying a thread, and takes up only as much space as its map of thread-local bindings.

Based on discussion on clojure-dev, I have attached an alternative patch ({{CLJ-1085-refactor.diff}}), which:

• breaks out the libspecs specifying the implicit refers into their own var (so that they can be consistently applied by other REPL implementations)
• moves the default require of the libspecs to be invoked only when REPL is started from clojure.main

Screened and liked the second "refactor" patch.

I don't believe this is Major priority, but I cannot edit the ticket after having created it.

This patch makes PersistentVector$ChunkedSeq implement Counted.

Performance before:

(let [v (vec (range 10000))
      vs (seq v)]
  (time (dotimes [_ 10000]
          (count v)))
  (time (dotimes [_ 10000]
          (count vs))))
;"Elapsed time: 0.862259 msecs"
;"Elapsed time: 7228.72486 msecs"

Performance after (with the patch):

(let [v (vec (range 10000))
      vs (seq v)]
  (time (dotimes [_ 10000]
          (count v)))
  (time (dotimes [_ 10000]
          (count vs))))
;"Elapsed time: 0.967301 msecs"
;"Elapsed time: 0.99391 msecs"

Also with Paul's test case.

Before:

(time (dotimes [_ 10000000] (object-array [1])))
;"Elapsed time: 1668.346997 msecs"
(time (dotimes [_ 10000000] (object-array (rseq [1]))))
;"Elapsed time: 662.820591 msecs"

After:

(time (dotimes [_ 10000000] (object-array [1])))
;"Elapsed time: 757.084577 msecs"
(time (dotimes [_ 10000000] (object-array (rseq [1]))))
;"Elapsed time: 680.602921 msecs"

Two patches to be applied together, the 10/19 patch adds tests and updates to latest test.generative.

As a workaround you can create a setter method for that field, e.g.

(set-a [this, na] (set! a na))

Is this the same issue as CLJ-1023?

That one looks very similar, yes.
The "non-tail try-catch as closure" being the problem, would explain the examples above, as well.

closing as duplicate of CLJ-1023

Screened.

Looks fine to me.

Clojure-dev discussion: https://groups.google.com/d/topic/clojure-dev/ME3-Ke-RbNk/discussion

Attached patch 001-make-PersistentQueue-hash-match-equiv.diff, 15/Sep/12.

Attached 002-make-PersistentQueue-hash-match-equiv.diff, 15/Sep/12. This patch supercedes 001-make-PersistentQueue-hash-match-equiv.diff.

Replaced test code which calls to Util/hasheq with code which calls hash instead.

This patch has a minor conflict with my patch for CLJ-1059, since they both add an interface to PersistentQueue (IHashEq here, List for CLJ-1059).

Thanks for the patch, Philip, it looks good to me.

It really is a pity the implementations of hashCode and hasheq are duplicated. I wonder how important it is to extend Obj? Regardless, that's the approach PersistentQueue was already taking, so changing that would be outside the scope of this ticket anyway.

I can't apply this patch with "git am", but "patch -p 1" works fine. I'm hoping this is a configuration problem on my end, so I'm marking this screened.

This better belongs as a ticket for the JIRA project specifically for data.priority-map. It is here: http://dev.clojure.org/jira/browse/DPRIMAP

I have filed a ticket DPRIMAP-2 for that project, and will close this one.

You can find a complete list for all projects here: http://dev.clojure.org/jira/secure/BrowseProjects.jspa#all

This better belongs as a ticket for the JIRA project specifically for algo.monads. It is here: http://dev.clojure.org/jira/browse/ALGOM

I have filed a ticket ALGOM-6 for that project, and will close this one.

You can find a complete list for all projects here: http://dev.clojure.org/jira/secure/BrowseProjects.jspa#all

At first glance, it appears that there could be some code sharing here. But the attached patch changes the semantics of run, which is a non-starter.

The only thing that changes is the type of thrown exception.

Current call tree:

Keyword.run() -> throw new UnsupportedOperationException()

Call tree with patch applied:

Keyword.run() -> AFn.run() -> AFn.invoke() -> AFn.throwArity(0) -> throw new ArityException(...)

(I.e. Keyword.run() always throws an exception, with and without my patch.)

$ java -version
java version "1.7.0_03"
OpenJDK Runtime Environment (IcedTea7 2.1.2) (7u3-2.1.2-2)
OpenJDK 64-Bit Server VM (build 22.0-b10, mixed mode)
$ lein version
Leiningen 2.0.0-preview10 on Java 1.7.0_03 OpenJDK 64-Bit Server VM

That's really a nasty problem. I wrote the code that makes it possible to compile clojure with either a JDK7 and native ForkJoin or an older JDK + jsr166y. Unfortunately, if you build clojure with a JDK7, reducers' fold requires a JDK7 at runtime, and if you build clojure with a JDK <7 + jsr166y, fold also requires jsr166y at runtime.

The essential problem is that clojure is AOT-compiled to class-files and those are put into the release jars. Ordinary clojure libraries are distributed as jar files containing clj files that are compiled when loaded. There, the implemented approach works just fine. For example, again your example with 1.5.0-alpha4:

;; 1.5.0-master4 compiled with JDK6 + jsr166y running on a JDK7
user=> (require '[clojure.core.reducers :as r])
nil
user=> (def v (vec (range 10000)))
#'user/v
user=> (r/fold + (r/map inc v))
ClassNotFoundException jsr166y.ForkJoinTask  java.net.URLClassLoader$1.run (URLClassLoader.java:366)
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Now load the reducers.clj source code again, so that it
;; picks the right ForkJoin-impl for the current platform
(load-file "/home/horn/Repos/clj/clojure/src/clj/clojure/core/reducers.clj")
nil
user=> (r/fold + (r/map inc v))
50005000

So IMO, the best thing we can do is to omit AOT-compilation for reducers but to include only its clj file so that it'll be compiled automatically on the platform it eventually runs.

This patch removes the clojure.core.reducers namespace from the namespaces to be AOT compiled. So now this namespace will be JIT-compiled when being required, and at that point either the JDK7 ForkJoin classes or the jsr166y classes need to be there.

Rich: what is the right approach here?

I think that clj-1065-set-map-constructors-allow-duplicates-v1.txt dated Sep 7 2012 updates Clojure for the behavior Rich recommends on the dev Wiki page in the description. I may have missed something, though. Perhaps the most questionable part is the way I've chosen to implement array-map always taking the last key's value. It is no less efficient than the PersistentArrayMap createWithCheck method, i.e. O(n^2).

Thanks!

array-map is ok. I would prefer that the docs strings say "as if by repeated assoc" (or conj for sets). "eliminates" and "last" are less precise e.g. what if you pass equal things, but with different metadata, to hash-set? "Eliminates dupes" doesn't say.

clj-1065-set-map-constructors-allow-duplicates-v2.txt dated Sep 8 2012 supersedes yesterday's -v1.txt patch, which I will remove.

This one updates doc strings per Rich's suggestion.

Also, his mention of metadata and "as if by assoc" led me to beef up the new test cases to check that metadata is correct, and I thus found that my new array-map constructor was not doing the right thing. This one does.

There is still no implementation of Rich's #3 yet. Just wanted to get this one up there in case someone else builds on it before I do.

Patch clj-1065-do-map-constant-duplicate-key-checks-compile-time-only-v1.txt only makes changes that should eliminate run-time checks for duplicate map keys, if they are compile-time constants.

It is currently separate from the changes to those for the set/map constructor functions, since I am less sure about these changes. I'm not terribly familiar with the compiler internals, and I might be going about this the wrong way. Better to get separate feedback on these changes alone. I'm happy to merge them all into one patch if both parts look good.

I think passing in an empty vector should rather be considered an error, since the doc specifies that `ks should be a sequence of keys`.

The same wrong (in my opinion) behaviour is shown by assoc-in

user=> (assoc-in {} [] 1)

Unknown macro: {nil 1}

Specifying an empty vector is no error in case the vector is not manually specified but computed as in the scenario I mentioned above.
"(apply f m args)" in case of an empty vector is the natural continuation of the usual behavior of update-in:

• update-in with 2 keys the second level map is updated
• update-in with 1 key the first level map (child of given map) is updated
• update-in with no key the given map (zero level) is updated.

Otherwise, you will always have to wrap update-in in something like the following when the keys vector is computed:

(if (seq keys) (update-in m keys f args) (apply f m args))

I believe this is a duplicate of CLJ-373. Please add your comments and/or suggested patches to that ticket rather than this one. This one should likely be closed as a duplicate. Let me know if you can think of any reason why this one covers new ground that CLJ-373 does not.

Yes, you are right - it is a duplicate.

Closed, as it is a duplicate of CLJ-373.

To be more correct: it breaks compilation of namespaces that require/load such ns without any public functions.

An example project that reproduces the issue (see in the README):
https://github.com/michaelklishin/clj1062

Patch applied.

Mac OS X 10.8.1, Java 1.7.0_06

Screened.

Thanks!

It is unusual in Clojure to expect/rely on concrete list? checks, particularly given the prevalence of seq?

If the behavior documented here is causing problems, please discuss use case on mailing list, and then we can open a ticket.

Patch clj-1052-assoc-should-throw-exc-if-missing-val-patch1.txt dated Aug 29 2012 makes assoc throw an IllegalArgumentException if more than one key is given, but the last key has no value. It includes a few simple test cases with correct and incorrect arguments to assoc.

IMO if the error message included something like "assoc expects even number of arguments after target, found odd number", or some mention of the number of arguments the error would be clearer to me.

clj-1052-assoc-should-throw-exc-if-missing-val-patch2.txt is identical to the now-removed -patch1.txt, except for the text of the exception thrown, updated as per Ambrose's suggestion.

The patch appears correct. It does introduce a single extra (next) per iteration into the success path, although that seems unlikely to dominate the work. Wouldn't hurt to add as assessment showing this is no slower for correct programs.

Test platform: Mac OS X 10.6.8 + Oracle/Apple Java 1.6.0_35 Java HotSpot(TM) 64-Bit Server VM

With latest Clojure master as of Sep 19 2012:

Clojure 1.5.0-master-SNAPSHOT
user=> (set! unchecked-math true)
true
(defn count-maps [n]
(let [base {:a 1}]
(loop [i n
sum 0]
(if (zero? i)
sum
(let [m1 (assoc base :b 2 :c 3 :d 4 :e 5 :f 6 :g 7 :h 8 :i 9 :j 10)]
(recur (dec i) (+ sum (count m1))))))))
#'user/count-maps
user=> (time (count-maps 10000000))
"Elapsed time: 48784.077 msecs"
100000000
user=> (time (count-maps 10000000))
"Elapsed time: 49028.52 msecs"
100000000
user=> (time (count-maps 10000000))
"Elapsed time: 50314.729 msecs"
100000000

Same Clojure version, plus the patch that was screened:

user=> (time (count-maps 10000000))
"Elapsed time: 49576.191 msecs"
100000000
user=> (time (count-maps 10000000))
"Elapsed time: 49957.866 msecs"
100000000
user=> (time (count-maps 10000000))
"Elapsed time: 52149.998 msecs"
100000000

(average of 3 times after patch) / (average of 3 times before patch) = 1.0240

So 2.4% slowdown on average for that test case. I should add that I'm not a statistician, but note that this 2.4% difference is less than the variation in run time from one run to the next of the same experiment. Likely any real statistician would recommend collecting a lot more data before asserting there is a change in performance.

clj-1052-assoc-should-throw-exc-if-missing-val-patch3.txt dated Oct 5 2012 is the same as the previous patch clj-1052-assoc-should-throw-exc-if-missing-val-patch2.txt dated Aug 29 2012 except some context lines have been updated so that it applies cleanly using git. The older version will be removed in a minute.

OSX

this is not a bug in clojure.

declare creates an unbound var then your def tries to use the value of the var before you have given it a value.

declare does not let you use a value of a var before you have given it one (via def or whatever).

meta comment:
I would close this as "Declined", but I am not sure if that is kosher for me to do.

Thanks for looking into this.

Just want to point out that as far as the declare and the use of expr as a forward reference, the second scenario I've presented (with factor*) uses (declare) the same way, yet it works: the var "expr" in (factor*) ends up pointing to the root value set later, but before it runs.

Seems to me similar to this case, where f is defined in terms of itself and it works fine, having the var "f" obtained its root binding after the def form runs:

user=> (def f (fn [n] (if (< n 1) 1 (* n (f (- n 1))))))
#'user/f
user=> (f 6)
720

Given that I've got a usage that works, I would suspect that there's something about the first case that prevents the root binding to be visible to the declared var.

the formatting of the second case makes it hard to read, but it looks like expr is referenced inside a function, so the var #'expr will not be derefed until the function is run.

Though the difference isn't quite apparent to me, I kind of grasp the idea that the var may be in one case deref'ed earlier in one case than the other because calls to (bind) are eager and in the case of the additional call to (between) the var is inside the function. I'll ask the board for advice on this and this ticket can be closed. Thanks.

Closed based on discussion in comments.

Please close and reject. The patch is not working if val has non final fields.

Note that this patch also removes the cyclic load tests. Those tests relied on leaving broken code on the test classpath, which is very hostile to tooling (in this case the test runner's ability to walk the code in the test directory.)

I believe such tests are an antipattern, and should be implemented differently or e.g. placed in an ancillary test suite.

I agree with you position on the cyclic load tests. The patch looks sane to me and ran without hitch. A couple points of note:

• This requires one to re-run antsetup – not a problem, but it might trip a few people
• The final output of the test run is different than before. In the case of a successful run we now see only the generative success stats. Is the regular clojure.test stats included? It's not clear to me, but I don't think they are. In the fail case we see something like the following. I actually like the map output better, but one advantage to the stack trace output was that it sometimes helped identify code bugs. In any case, it would be nice to see an aggregate score at the end. This is a test.generative feature request I know.

     [java] {:clojure.test/vars (test-equality),
     [java]  :thread/name "main",
     [java]  :pid 8682,
     [java]  :thread 1,
     [java]  :type :assert/fail,
     [java]  :level :warn,
     [java]  :test/actual (not (not true)),
     [java]  :test/expected (not (= nil nil)),
     [java]  :line 28,
     [java]  :tstamp 1345649256308,
     [java]  :file "data_structures.clj"}

In all, I like this! Now we need to start thinking up more specs.

The stats reports are aggregate, i.e. they rollup both the generative and clojure.test stats. It would be straightforward to separate these in the report. I will look at doing that in the next drop of c.t.g.

The output already includes stack traces for errors, but not for test failures. I thought this was consistent with clojure.test, but maybe I missed something.

Thanks for the clarification Stu. You're right that the exception behavior is consistent, the confusion was my own.

Patches have been applied as of September 1, 2012.

Ian, thanks for the patch. It is Rich Hickey's policy only to consider applying patches to Clojure from those who have signed a Clojure contributor agreement: http://clojure.org/contributing

Were you interested in doing so, or perhaps it is already in progress?

I wasn't aware that this was necessary. I'm mailing the form in.

Patch clj-1042-negative-subs-patch2.txt dated Aug 27 2012 is identical to Ian Eure's negative-subs.patch, except it is in the desired git format.

Ian, for future reference on creating patches in the desired format, see the instructions under the heading "Development" on this page: http://dev.clojure.org/display/design/JIRA+workflow

Thanks, will do.

If Clojure decides to support Python-style negative indices, you should also consider adding support to subvec.

Patch extended to support negative indices on (subvec) as well.

The arg to rindex should probably be tagged with ^clojure.lang.Counted instead of ^String now.

Regarding the previous comment, String is a Java class so it isn't a clojure.lang.Counted. Is the type hint necessary? Maybe it should be on the call rather than the defn.

Ignoring the type hinting, I'll suggest a slightly simpler way to implement the rindex logic:

(defn rindex [coll i]
(if (neg? i) (+ (count coll) i) i))

In any case, I'm not sure rindex should be public even if you want the subs and subvec enhancements. Someone needs to make the case for adding a new function to core.

The Pythonic negative index is a debatable feature since it's pretty easy to implement for yourself if you want it.

Sorry, the type hint on rindex args isn't necessary at all. Just looked up in the source, calling count should never be reflective, since (count ..) emits calls to clojure.lang.RT/count.

Your solution looks good.

Negative indices were considered and rejected a long time ago. (I am merely conveying information--I have no strong opinion on this one.)

Note: If some people really like negative index behavior as in Perl or Python, it is straightforward to create a library of functions in a different namespace, perhaps with different names, that can do it. Perhaps a "pythonisms" library?

Would this be accepted as part of clojure.string instead? I considered putting it there, but thought it would be confusing to have multiple substring functions in different namespaces.

This is very helpful in practice, and I'd really like to see at least the (subs) stuff in Clojure.

Disclaimer: I'm no Clojure/core member, so can't speak authoritatively on whether something would or would not be accepted into clojure.string.

However, given that clojure.string is distributed with clojure.core, my guess is it would not be accepted. You'd be able to get things like this out for you and others as a separate library distributed on Clojars. That would also make it easy to include other Python-like things that you don't find in Clojure already.

This isn't about "python-like things," this is about a useful feature. Lots of languages support this: Perl, PHP, Ruby, Python, JavaScript, to name a few. Are you really suggesting that I should create a whole package for a version of a function in clojure.core with slightly different semantics? That's insane.

Anyway, I'm done wasting my time trying to navigate this hopelessly broken process. Maybe it would have been accepted if I included yet another way to interoperate with Java types.

Stuart, do you remember why specifically negative indexes were rejected? Developing a separate library for a minor improvement to an existing function sounds unreasonable.

some explanation about this topic was given by Rich Hickey himself here: http://news.ycombinator.com/item?id=2053908

citation:
"...Yes, there is a backlog from when it was just me, and it will take a while to whittle down. We have finite resources and have to prioritize. I can assure you we have more important things to concentrate on than your negative index substring enhancement, and are doing so. You'll just have to be patient. Or, if you insist, I'll just reject it now because a) IMO it's goofy, b) you can make your own function that works that way c) we don't get a free ride from j.l.String, d) it begs the question of negative indices elsewhere..."

i've been following this thread hoping this feature would be included. but whatever the reason was for the rejection, i'm sure it was thoughtful. great thanks for this wonderful language, and thanks Ian Eure for his effort.

That HN link eventually leads back to CLJ-688 which was rejected.

Michael: A proposal for negative indexes would need to be systematic in considering implications for all functions that have index arguments.

Ian: Clojure is driven by design, not incremental piling of features.

All: clojure.string is incomplete in more important and fundamental ways than negative indexes. This sucks now, and will suck worse as more code is written in different dialects. I find myself wishing string was a contrib, so we could iterate faster.

Stuart: Any specific proposals for how you'd like to see clojure.string improve? If it can be made a contrib, that would be cool, but understood if that would be considered too breaking of a change. Even if it isn't made a contrib, having tickets for improvement ideas you are willing to see means patches might get written, and they'll get in some time.

Patch applies cleanly against 4004d267e124f12b65b0d7fb6522f32a75e3c4fb. Submitter confirmed on the CA list.

Stuart, your patch doesn't check for Reduced after calling f on the current node; I think you need to move that check along with the call to f.

CLJ-1041 is a related issue, and its patch also includes a commit that I think would fix your patch.