Before being considered, this ticket needs:

• a good problem statement
• an assessment of where existing code calls these functions with something other than sets
• a table of alternatives and their tradeoffs. presumably alternatives are: add specs, add validation checks, add coercions, etc. Tradeoffs may include effects on existing callers (known or unknown), performance, etc.

Decisions to make:

• Are existing calls (with inputs that are not sets) valid or invalid?
• What change should be made in the functions

Patches are probably not super useful until those things are decided.

-4 patch removes unnecessary call to symbol (and squashes), retains attribution

-5 patch clarifies docstring and guards against illegal arguments

-2 fixes a bug that lose the phase in ex-triage

clojure 1.9

Added a patch that supports java.nio.file.Path in Coercions and IOFactory. There are more places (particularly the do-copy multimethod) that might warrant Path support, but this seemed like a good start.

Note, in case there's any confusion, I have signed the CA (June 17th), though my name is not yet on the site's list of CA signers.

java.nio.file.Path was added in Java 7, which is perhaps why this change didn't make it in before. Clojure 1.10 will require Java 8, so perhaps it's time to consider this patch.

Thanks Daniel – I edited the repro case a bit.

This is due to the Compiler$CompilerException.toString() change in CLJ-2373, looking at it.

-2 fixes some test failures - oops!

Fixed in https://github.com/clojure/clojure/commit/d585030f0e12da9351136d6cf886d358942c3b3f.

Tested against clojure 1.7 and 1.8

This macro definition is incorrect as it's passing 4 args from one arity to the other, not 2 (which is what the error tells you). The "workaround" fixes that problem. I don't see a bug here.

This is clearly a bug, since multi-arity macro definition requires passing two additional arguments (&env and &form), which are usually hidden, to the macro.

See more here: https://stackoverflow.com/a/25569059/6879054

No, this is definitely not a bug and the original definition is wrong, as is the stackoverflow answer you posted.

The "workaround" is the only correct way of writing that macro. The original defininition happens to work due to an implementation detail of how macros are implemented in clojure, but is simply incorrect.

Thank you for clarifying that.

The fspec is the spec in question in here and it does have a component :args (the fspec instance supports key lookup via ILookup for :args as well). So while I would like to improve the error message and data here, I don't agree with removing :args from path. One thing that I think would be useful is for an instrumentation failure to better state the invocation (combining the function and the args into the original call). Right now those are separated and it take some mental work to knit the arg list back together.

So while I would like to improve the error message and data here, I don't agree with removing :args from path.

Yes, so once we decide to go with Solution 2., then I think there is no need to remove :args from :path.

One thing that I think would be useful is for an instrumentation failure to better state the invocation (combining the function and the args into the original call).

I totally agree that would be useful, though it sounds like it's somewhat beyond the scope of the ticket in terms of consistency improvement.

I've made a patch just to express what I meant. Any feedback will be appreciated.

One thing that I think would be useful is for an instrumentation failure to better state the invocation (combining the function and the args into the original call). Right now those are separated and it take some mental work to knit the arg list back together.

Apologies if I'm missing something, but is it even possible to get the function from the explain-data? I don't see it in explain-data, but perhaps it is recoverable somehow? In any case, I would greatly appreciate it if this type of information was available, since it would make it possible to print clearer error messages.

Hey, is this already done by CLJ-2392?

The JLS infers lambda types by hunting for matching functional interfaces AKA "Single Abstract Method" classes [1] (whether they're interfaces or abstract classes.) We could have a reify-like helper that detects these classes [2]. You would have to hint the target class. We don't really need things that are both `IFn` and `j.u.f.Predicate`, etc.

(import '[java.util.function Predicate Consumer])

(let [orig [1 2 3]
      st (atom [])]
  (.forEach orig (jfn Consumer [x] (swap! st conj x)))
  (= @st orig))

[1] https://docs.oracle.com/javase/specs/jls/se8/html/jls-9.html#jls-9.8
[2] spike https://gist.github.com/ghadishayban/0ac41e81d4df02ff176c22d16ee8b972

Well, that would be an improvement. The practical problem I run into is that I'm frequently deep in a fluent interface, and don't necessarily know the exact class. That said, it's usually only in a few places. Would it make sense to have a registry? Perhaps something like:

(auto-infer-lambda [java.util.function, org.apache.kafka.streams.kstream])

Do you ever use SAM classes that are abstract classes and not interfaces?

Here's an alternative approach in my ike.cljj library. This uses MethodHandles (i.e. java.lang.invoke package) instead of regular reflection. I'm not sure if I tested this on abstract classes yet.

The usage looks similar to what Ghadi posted:

(defsam my-sam
  java.util.function.Predicate
  [x]
  (= x "it matched"))

(-> (Stream/of "not a match" "it matched")
    (.filter my-sam)
    (.collect Collectors/toList)

(-> (IntStream/range 0 10)
    (.filter (sam* java.util.function.IntPredicate odd?))
    (.collect Collectors/toList)

It uses MethodHandleProxies.asInterfaceInstance to create a proxy instance of the interface that calls a method handle calling a Clojure function. It doesn't try to validate parameter counts, it just treats it as varargs delegating to IFn.applyTo(ISeq). Not sure if it's the most efficient but it was effective for my needs.

I think the LambdaMetaFactory may be the preferred way to meet this type of use case. It was harder for me to follow exactly how to use that though so I didn't end up looking to deep into it.

The main functional issue I have with my approach (and Ghadi's) is that you have to explcitly provide the interface you want to proxy. Java lambdas and Groovy Closures can be used against methods that expect a SAM and it just coerces based on what the method expects. Ideally this would be supported by Clojure too.

Instead of having to do:

(-> (IntStream/range 0 10)
    (.filter (sam* java.util.function.IntPredicate odd?))
    (.collect Collectors/toList)

I'd like to do this:

(-> (IntStream/range 0 10)
    (.filter odd?)
    (.collect Collectors/toList)

Another possible avenue is to extend java.util.function.Supplier to Clojure functions with an explicit 0-arity. That interface is becoming increasingly common in practice; it might be valuable to special-case it. (We shouldn't & couldn't do a similar thing for defrecords, as they already have a method called get, which clashes with Supplier's only method.)

Moving to Java 8 as a baseline allows us to use default interface methods.

The `some-java-fns-interface.patch` patch implements Consumer, Function, Predicate and Supplier on IFn.

If you want to go this route, I would be very happy to implement all interfaces under java.util.function on IFn as well as the accompanying tests. I currently use this code to play with FoundationDB through its Java client and it works well for me.

https://github.com/marctrem/clojure/commit/97742493f674edd8f6c034ee94da84fa62a76bad

If I could just use IFn's anywhere a java.util.function.* was needed, that would be fantastic!

-2 patch adds some new function meta, some doc string changes in ex-triage, and handles the case where the JVM omits the stack.

-3 patch has some fixes in using meta :file and :line annotations

-4 has some more subtle fixes in supporting :file/:line meta

Alex, you may have forgotten the patch

yup

The original patch (clj-2417.patch) is missing the '(is ..' wrapping in the test, without which the test are run but failures not reported. clj-2417-fix-test.patch adds the missing wrapping as a separate patch. Another option is to fix this in the original patch and to re-apply it.

The patch has already been applied, can you make a new ticket?

Test fix as new ticket in https://dev.clojure.org/jira/browse/CLJ-2424

{{CLJ-1097.diff}} contains a fix for this issue, as well as a separate commit that eliminates a series of casts in order to improve readability in the area.

Chas, your patch doesn't apply cleanly to latest Clojure master as of Oct 5 2012. I'm not sure, but I think some recent commits to Clojure on Oct 4 2012 caused that. I also take it as evidence of your awesomeness that you can write patches for tickets that haven't been filed yet

"patches for tickets that haven't been filed yet?"

Anyway, tweaking up this patch is a small price to pay for having column meta. New {{CLJ-1097.diff}} patch attached, applies clean on master as of now. Otherwise same contents as in the original patch, except:

• the same dynamic is also applied to :column metadata, now that it's available
• the changes have been rebased into a single commit (including the elimination of the casts in MetaReader, which were becoming so numerous that the code was less readable than most

"patches for tickets that haven't been filed yet?"

He was referring to the fact that you uploaded "CLJ-1097.diff" while the ticket is #1079

Oh, hah! Twice now, even! One more data point recommending my having slight dyslexia or somesuch. :-P

I've replaced the attached patch with one that is named properly to avoid any later confusion.

Refreshed patch to apply cleanly to master after the recent off by one patch for :column metadata.

This feels backwards to me. If a special purpose tool wants to convey information via metadata, why does it use names that collide with those used by LispReader?

The information being conveyed is the same :line and :column metadata conveyed by LispReader — in fact, that's where it comes from in the first place.

Kibit (and cljx) is essentially an out-of-band source transformation tool. Given an input like this:

(ns com.foo.hosty)

(defn ^:clj system-hash
  [x]
 (System/identityHashCode x))

(defn ^:cljs system-hash
  [x]
  (goog/getUid x))

…it produces two files, a .clj for Clojure, and a .cljs for ClojureScript. (The first code listing in the ticket description is the former.)

However, because there's no way to transform Clojure code/data without losing formatting, anything that depends on line/column numbers (stack traces, stepping debuggers) is significantly degraded. If LispReader were to defer to :line and :column metadata already available on the loaded forms (there when the two generated files are spit out with *print-meta* on), this would not be the case.

clj-1079-patch-v2.txt dated Feb 7 2013 is identical to Chas's CLJ-1079.diff dated Oct 7 2012, except it applies cleanly to latest master. I believe the only difference is that some white space in the context lines is updated.

Sorry for the noise. I've removed clj-1079-patch-v2.txt mentioned in the previous comment, because I learned that CLJ-1079.diff dated Oct 7 2012 applies cleanly to latest master and passes all tests if you use this command to apply it.

% git am --keep-cr -s --ignore-whitespace < CLJ-1079.diff

I will update the JIRA workflow page instructions for applying patches to mention this, too, because there are multiple patches that fail without --ignore-whitespace, but apply cleanly with that option. That will eliminate the need to update patches merely for whitespace changes.

Why do we care how fast it is to create a Throwable?

I use ExceptionInfo as a "fail" container in https://github.com/dawcs/flow, but I worry a bit about performance, cause creating ExceptionInfo is costly(due to filling stacktrace). In most cases where it's used now, we don't need stacktrace for ExceptionInfo, so I thought it may be good case for optimization.

We use ExceptionInfo stacks all the time so I don't buy that, and I think the case where optimization would affect anything is rare. I'm going to decline this - I don't think it's worth complicating the api for this.

Clojure 1.10.0-beta4

The first one here is an inlined function and happens during compilation, so is a little different than the others. Because it's inlined, it's not spec-able. The name parts there are generated within the compiler as well. This one might be fixable based on the regularity of the name construction, although I don't see it as a particularly high priority.

The latter two actually will be a bit different once we apply CLJ-2420:

user=> (assoc)
Execution error (ArityException) at user$eval154/invokeStatic (REPL:1).
Wrong number of args (0) passed to: clojure.core/assoc--5406
user=> (dissoc)
Execution error (ArityException) at user$eval156/invokeStatic (REPL:1).
Wrong number of args (0) passed to: clojure.core/dissoc

Changes here are actually eliding all the internal frames and reporting the top user frame now (here, it happens to be in the REPL invocation, but if it was in a user namespace, you'd see that instead).

I guess in both cases, the numbers you're seeing are a standard technique used throughout Clojure so the question might be whether it's possible to address this in a systematic way. Functions don't inherently carry their unmunged name right now, so demunging from class name is never going to be a foolproof mechanism.

Worth thinking about how to do this better, but not something I expect to look at soon.

It's not just rename-keys, there are a few fns in clojure.set that don't make sense being there, at first glance anyway.

It certainly harms discoverability of these fns.

I've come across at least one re-implementation of rename-keys and map-invert during my day job because the author didn't know these fns exist.

I'd argue for breaking the relational and map fns out of clojure.set into their own namespaces and deffing some vars in clojure.set for backwards compatibility.
Those compatibility vars could be deleted in 1.10.

I'm happy to do this (or another approach) but would like some buy in on the approach from the core team first.

I think this is very, very unlikely to ever happen

We will not delete/move existing vars as this would break existing programs.

> We will not delete/move existing vars as this would break existing programs.

This issue can be addressed without deletion as per Gordon's suggestion:

A new var could be added to clojure.core named clojure.core/rename-keys. Then the rename-keys var in clojure.set can be defined as:

(def rename-keys clojure.core/rename-keys)

I am aware of that, which is why I did not close the issue. I was just stating one possible resolution that is off the table.

Out of curiosity - does anyone know how/why those functions ended up in the `clojure.set` namespace?

P.S. If someone decides to alias things between namespaces I'd suggest creating a `clojure.map` ns over pouring more stuff into core.

a more accurate name for clojure.set based on its contents would be something like clojure.relational-algebra, where it defines something like relational algebra over sets of maps instead of over sets of tuples. A common operation in that case is renaming keys in maps (similar to using AS in a sql query). Which explains why rename-keys is in clojure.set

Bozhidar - only a guess, not knowledge. Relations are sets of records/tuples, so to me it makes some sense that functions on relations are in clojure.set. rename-keys is probably there because it is used by two of the functions operating on relations.

1.9.0-alpha11

In this case, it's considering the s/* to be a non-match and then matching the (Box. 0) against (s/coll-of integer?). This matches the initial predicate (coll?) but falls through to the :else case which presumes it can call `empty`.

You can work around it by adding a :kind predicate to the coll-of:

(s/conform
        (s/cat :boxes (s/* #(instance? Box %))
               :name (s/coll-of integer? :kind #(not (record? %))))
        [(Box. 0) [5]])
;;=> {:boxes [#user.Box{:a 0}], :name [5]}

But why does it need to do empty in the first place?

Does this mean that you can't check collections of records in spec?

I ran into this myself. Conforming a record to a coll-of spec should show a normal spec error, not throw from deep inside spec's implementation.

Sorry, not going to do that. You don’t have to alias it at all.

fn-tail here means "additional args as could be passed to fn", which does include the function name, so that is supported.

Adding patch

Rather than changing the docstring from multimethod to multifn, I would change the code from multifn to multimethod. Otherwise looks good.

Adding updated patch that changes the argument name to match the docstring, rather than the other way around.

I'd prefer only the docstring to change here for the dispatch-val (so docstring changing to match the code).

Patch that changes only the docstring.

I've not changed the word multimethod to multifn in the docstring. I believe that even though the parameter is not documented, the name multimethod is more clear.

Can you elaborate on why you want to change the docstring rather than the argument name please Alex?

I changed my mind and I think it would be better to just change the docstring to match the arg names.

So, it's kind of interesting if you look into the details of this. Technically, at the point where the exception occurs, the defmulti is not on the stack (that's the dispatch function, which has already completed), but the defmethod is.

defmethods are actually defined as fn's and are typically treated as anonymous fns, although it's actually possible to name them independently:

user=> (defmulti dispatcher (fn [item] (:type item)))
#'user/dispatcher
user=> (defmethod dispatcher "cat" cat-method
         [{:keys [lives]}]
         (/ lives 0))
#object[clojure.lang.MultiFn 0x6e1d4137 "clojure.lang.MultiFn@6e1d4137"]
user=> (dispatcher {:type "cat" :lives 9})
ArithmeticException Divide by zero  clojure.lang.Numbers.divide (Numbers.java:163)
user=> (pst *e 5)
ArithmeticException Divide by zero
	clojure.lang.Numbers.divide (Numbers.java:163)
	clojure.lang.Numbers.divide (Numbers.java:3813)
	user/eval155/cat-method--157 (NO_SOURCE_FILE:11)         ;; <== cat-method
	clojure.lang.MultiFn.invoke (MultiFn.java:229)
	user/eval161 (NO_SOURCE_FILE:12)

I think what would be best is if the defmethod function defaulted to a unique name based on the defmulti function name (if no name was specified).

CLJ-2212 is related

Oh, interesting.

The symbol that identifies the multimethod (dispatcher) in this case appended with some sort of string representation of the dispatch value would be a really nice default value here.

So you might have functions named
`dispatcher-default`, `dispatcher-cat`, etc.

I assume that you might have to add some additional uniqueness suffix on the end, too.

clj-1103-make-conj-assoc-dissoc-handle-args-similarly-v1.txt dated Nov 4 2012 makes conj conj! assoc assoc! dissoc dissoc! handle args similarly to each other.

I too ran into this and started an additional discussion here: https://groups.google.com/d/topic/clojure-dev/wL5hllfhw4M/discussion

In particular, I don't buy the argument that (into coll xs) is sufficient, since into implies conj and there isn't an terse and idiomatic way to write (into map (parition 2 keyvals))

So +1 from me

Patch clj-1103-2.diff is identical to the previous patch clj-1103-make-conj-assoc-dissoc-handle-args-similarly-v1.txt except it applies cleanly to latest master. The only changes were some changed context lines in a test file.

Patch clj-1103-3.diff is identical to the patch clj-1103-2.diff, except it applies cleanly to latest master. The only changes were some doc strings for assoc! and dissoc! in the context lines of the patch.

Patch clj-1103-4.diff is identical to the previous clj-1103-3.diff, except it updates some context lines so that it applies cleanly to latest Clojure master as of today.

Can someone update the description with some code examples? Or drop them here at least?

What do you mean code examples?

These currently work as expected:
(dissoc {})
(disj #{})

The following fail with arity errors:
(assoc {})
(conj {})

Similarly for the transient ! versions.

This is annoying if you ever try to do (apply assoc m keyvals)... it works at first glance, but then one day, bamn! Runtime error because you tried to give it an empty sequence of keyvals.

Patch clj-1103-5.diff dated Aug 6 2014 applies cleanly to latest Clojure master as of today, whereas the previous patch did not. Rich added 1-arg version of conj to 1.7.0-alpha1, so that change no longer is part of this patch.

Patch clj-1103-6.diff dated Aug 29 2014 is identical to the former patch clj-1103-5.diff (which will be deleted), except it applies cleanly to the latest Clojure master.

Patch clj-1103-7.diff dated May 25 2015 is identical to the former patch clj-1103-6.diff (which will be deleted), except it applies cleanly to the latest Clojure master.

Updated patch to apply cleanly to master

Updated patch

Is it intentional that clj-1103-9.patch leaves out the changes to assoc! that were in the earlier patches? I can create another updated one if so – just curious if there was a reason for the change.

That was not intentional at all, thanks for catching it, I'm honestly not sure what happened on my end to make that happen. I think this patch should be a correct update. Sorry about that.

No worries, and I thank you for keeping the patch current, even while keeping my name on it.

Hoping to get the conversation started on this in time for a 1.9-beta:

The rationale in my predecessor ticket, CLJ-2030, was to create a construct to allow for auto-aliasing in .cljc files, which I would like to submit as a requirement. The obvious place for cljs to declare an auto-alias is the ns clause.
So if we don't want to grow alias for that use case, I would propose to grow the ns clause with a declaration for keyword - aliases. Let's give it a working title of (:kwns-alias ..) for this comment.

:kwns-alias would be used to establish namespace aliases for ::qualified/keywords

One open question is, how :kwns-alias should interact with alias. i.e. whether the namespace of ::qualified/keyword should always expand to the same as the one of a qualified/symbol or if they should be allowed to differ. I'd argue they should always be the same, because of the rule of simplicity. That means, that

• alias will need to check if the sym is already in :kwns-alias and throw, if so
• :kwns-alias will need to also work on `qualified/keywords probably shouldn't have knws in its name any more

So what's a good name for :kwns-alias? :let maybe?

beta = feature complete, so this isn't happening in 1.9.

I think adding anything to ns is likely off the table, but until I learn more about what Rich has in mind, I can't really suggest anything more.

that's a pity. but maybe 1.10 will have a shorter release time ...

In any case, I'll be very interested in Rich's idea to do this outside of the ns clause and still have it fit well with clojurescript (or even clojure, for that matter, ...)
Do you have any rationale, why growing ns for this is a bad idea?

ns already does way too much stuff and we don't have any desire to make it do more.

Since ns is essential to how a namespace is set up in clojure, I'd imagine a fix for that problem (of ns doing too much) to imply some rather drastic changes to the state of the art. As we won't break ns, I'd imagine either some sort of ns2 or a way to do namespace setup outside of the ns clause. I'm curious either way.

Since I know, that Rich will not be hassled about his hammock time, I'll lay off this for now. I'll be sure to check back as the next round of alphas goes out. I also hope for some productive discussion in the meantime.

Is there any reason such a mechanism would be limited to keywords? I have a place in my code where it would be very useful to have such a feature for symbols.

Also, related: a way for pr-str (and similar) to be configured to use short versions of namespaces. Debug output can be awfully noisy, even with namespaced maps.

No particular reason - usually for stuff like this we make it work the same for any ident (keyword or symbol).

Java 9+

Should unnamed vars (eg created by with-local-vars) print to #clojure/var nil or throw an exception? (exception is the print-dup behavior)

I think the vartag2.patch has an issue in that the test for print-var-tagged in missing an assertion. I think it is supposed to have something like:

(is (and ...))

within the last let-binding.

Printing unnamed vars has little utility (nothing distinguishes them from each other without knowing their code context), seems like it would be fine to have #clojure/var nil or #clojure/var :unnamed

Could I help out on this? Happy to work on it. It would be very helpful for me, trying parse the the specs from the :args and :ret in fspec.

As you can see, there is an existing patch here with substantial work on it already (and some early review from Rich and Stu). The most useful help on this at the moment would be feedback on the gaps/open questions in it.

Also, I should mention that I do not expect this to be finalized and included until we have finalized the spec forms themselves as they are still subject to change.

In the current patch, keywords referencing specs are missing except for keys in `s/keys`. What would the right way to model the keywords? One way would be to add `qualified-keyword?` as fourth option to the or-spec of `::spec`. Than `::spec` could be used as spec for the spec arg in `s/valid?`, `s/conform`, `s/form` and others.

Yeah, that would probably be good.

Another observation: Currently s/and and s/or are allowed to contain no preds:

(defspec clojure.spec.alpha/and (s/* ::spec))
(defspec clojure.spec.alpha/or (s/* (s/cat :tag keyword? :pred ::spec)))

there should be either a clear semantic definition what an empty form will mean or we should just use s/+ here and require at least one pred. For the and-form, we could define that (s/and) is the same as (s/and any?) and the same as just any?. For the or-form it is difficult, because s/conform returns tagged results. So conforming any value with (s/or) will result in ::s/invalid which renders an empty or-form useless.

There is a clear semantic definition for these - they are the same as clojure.core/and and clojure.core/or. As far as I'm concerned, this is all correct.

Thanks. I see. The documentation in clojure.core defines that (clojure.core/and) returns true and (clojure.core/or) returns nil.

OS X

This looks to me like relatively low hanging fruit unless I'm missing something; assigning to myself.

Patch clj-1400-1.diff to Compiler.java.

With this patch the example would now look like:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

I'm not sure the if(namesStaticMember(sym)) [see below], and the 2nd branch, is even necessary. Just by inspection I suspect it is not.

[footnote]

public static boolean namesStaticMember(Symbol sym){
	return sym.ns != null && namespaceFor(sym) == null;
}

patch: code and test

I tested on an actual source file, and the exception message included the file/line/col info as desired:

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)

Patch clj-1400-1.diff dated Jun 26 2014 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch. See section "Updating Stale Patches" on this wiki page for some tips on updating patches: http://dev.clojure.org/display/community/Developing+Patches

Attached is an updated patch: "clj-1400-2.diff". I removed the stale patch.

Few comments to address:

• Compiler diff was using spaces, not tabs, which makes it harder to diff. I attached a -3.diff that fixes this.
• the call to namesStaticMember seems weird. The name of that method is confusing for this use. Beyond that, I think it's doing more than you need. That method is going to attempt resolve the qualified name in terms of the current ns, but I think you don't even want to do that. Rather you just want to know if the sym has a ns (sym.ns != null) - isn't that enough?
• In what case will the other error "Var doesn't exist" occur? In other words, in what case will lookupVar not succeed in creating a new var here? If there is no such case, then remove this case. If there is such a case, then add a test.

Agree with all three of your bullets. Attached is an updated patch, clj-1400-4.diff.

• I used tabs in Compiler.java
• After close inspection of call to lookupVar(...), I believe null is returned only in the case of exactly this ticket (the symbol having a non-null namespace which has not been loaded yet). So I've taken out the conditional and the 2nd branch.
• (Test is unchanged)

(properly named patch)

You could throw a CompilerException with the location of the problem instead (as the ticket description suggests).

Sorry, I should've mentioned because this wasn't obvious to me either (and in fact I forgot until just now): the RuntimeException is already caught and wrapped in a CompilerException.

I'm not sure which try-catch block within Compiler.java this is happening in, there are multiple. But you can see in the output that the exception is a CompilerException and the file|line|col info is there:

In the Repl...

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

...or in a source file

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)

Also, at the point at which the RuntimeException of this patch is being thrown, the source line and col params to CompilerException are not available, or at least not afaict.

I'll follow up on this patch later - Rich thought it was making too many assumptions. I think we validated many of those but need to double-check those.

Can you add a test for the improved functionality?

You bet. I tried to before submitting this but found no existing tests for map-indexed to expand upon. Given that, I decided to just start the conversation first. If you think this is a good addition, I'll find a place to stick the tests and add a new patch file.

Add two unit tests for map-indexed. One tests old behavior (single collection) and the second tests mapping across 3 collections.

There were no existing tests for map-indexed that I could see to expand upon (using git grep map-indexed src/clojure)

This feature would be very useful. Is there a reason that no one continued to work on this in almost 5 years?

Predates my involvement with Clojure so don't know but don't see any reason why not.

If a ticket has not been moved to the Vetted state by Rich Hickey, it is unknown whether it is of interest to be added to Clojure at all. The default answer to any change to Clojure, until there is a compelling argument to the core Clojure developers otherwise, is "no". That said, JIRA tickets tend to be left open rather than declined if there is no compelling argument for declining it.

For some more background, see here, in particular the "Clojure Governance and How It Got That Way" article linked from there: https://dev.clojure.org/display/community/Contributing+FAQ

For details on Vetted, and other JIRA ticket statuses used by the Clojure core team, see here: https://dev.clojure.org/display/community/JIRA+workflow

Voting on tickets (there is a "Vote" link near the top right of the page if you log in to your JIRA account first) can in some cases help get attention to them sooner than tickets without votes, but there are no guarantees on time lines.

I didn't try but I'm sure this patch no longer applies. Having a single combined patch that can be applied would be a useful first step.

I just uploaded a new patch for map-indexed that allows it to accept multiple collections. I took the exact same approach that map does, but with the addition of the index. It has better performance than the old patch from 2012. I also included more tests. Both the function update and the new tests are in "CLJ-1601 map-indexed and tests 8-5-2017.patch"

Also, map-indexed had 0 tests prior to me writing the patch. That seems a bit dangerous for a function in core to be untested.

It's been a little while since I uploaded the patch here and it was prescreened. Both this ticket and another that I created are awaiting vetting, and the FAQ says that I should tend to my tickets. Is there something else that I should be doing to tend to it and move it along?

I don't think we'll consider any more enhancements in 1.9 timeframe.

Sounds right.

org.clojure/spec.alpha "0.1.143"

All

0.1.123

The same problem happens with s/every.

Oh, sorry. I meant s/every-kv, not s/every.

BTW, after looking into things around this, I found some other spec macros were putting inconsistent forms of :pred in their explain data.

For example,

(s/explain-data (s/tuple integer?) []) => (clojure.core/= (clojure.core/count %) 1)
(s/explain-data (s/& integer? even?) []) => #function[clojure.core/integer?] (not a symbol)

I'll file that as another ticket later.

Issue https://github.com/alexanderkiel/phrase/issues/22 relates to this.

Thanks for logging - I've been working on an approach for this one but never got around to actually logging it.

See also http://dev.clojure.org/jira/browse/TCHECK-111 which may solve this directly?

This is probably http://dev.clojure.org/jira/browse/TCHECK-83, which is fixed on test.check master.

I'm going to leave this open pending a new release and dependency update to test.check, which I presume will address it.

Having trouble with the patch, in that, things that work at the REPL fail when executed via `mvn test`. Tracking down why is taking some time.

Initial patch; code works but mvn test fails and I haven't figured out why.

Thanks to Hiredman, was provided with insight that back ticks needed due to how Maven/Ant runs the tests. All tests now pass.

As far as I can tell, this is basically the same intent as CLJ-866 which was completed in Clojure 1.6. You can do this now with test-vars:

user=> (use 'clojure.test) 
nil 
user=> (def counter (atom 0)) 
#'user/counter 
user=> (defn setup [f] (swap! counter inc) (f)) ;; a :once fixture with state 
#'user/setup user=> (use-fixtures :once setup) {:clojure.test/once-fixtures (#object[user$setup 0x7106e68e "user$setup@7106e68e"])} user=> (deftest ex (println "counter =" @counter)) #'user/ex user=> (test-vars [#'ex]) 
counter = 1 
nil 
user=> (test-vars [#'ex]) 
counter = 2 
nil

I think there is some advantage to being able to run the tests using is symbol, not its var. Further, the change I've suggested also returns the same kind of data that `run-tests` does.

Some changes needed on this patch before I will prescreen it:

• Patch should be squashed to a single commit
• Commit message in patch should start with "CLJ-1908"
• Change run-test* to run-test-var
• The docstring for run-test-var should be: "Run the test in Var v with fixtures and report." Kill the "called from" sentence".
• The first sentence of the docstring for run-test should be: "Runs a single test in the current namespace." Remove "This is meant to be invoked interactively, from a REPL.". Last sentence is ok.
• In run-test, replace (ns-resolve ns test-symbol) with the simpler (resolve test-symbol).

Thanks for the input; I'll have an updated patch shortly.

Updated patch, squashed and reflecting all of Alex's comments.

Possibly older Clojure versions (but not verified).

I don't think anything but primitive longs or doubles are intended to be supported in loop/recur. Presuming that is correct, this would be the expected behavior.

The last major round of numeric/primitive refactoring made the decision to focus only on supporting primitive longs and doubles. One consequence of this is that primitive int loops are difficult to optimize - the main time I run into this is when working with Java interop in a tight loop on (for example) String, collection, or array operations (all of which are int-indexed).

Re unchecked-inc vs unchecked-inc-int, the primary reason to have these two variants is not performance but behavior. In particular, hashing operations often expect to get 32-bit int overflow semantics, not 64-bit int overflow semantics.

In summary, I think in the example given I would not write it with either int or unchecked-inc-int but with long and unchecked-inc if you are looking for best performance.

Alex Miller I don't think that's correct, as (AFAIK) it's only in fn params/returns that primitive types are supposed to be restricted to longs and doubles.
Note that for example, char, byte, boolean, short etc work just fine in both let and loop, while int and float work fine in let but not in loop.

This is caused by the following 4 lines in Compiler.java https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L6278-L6281

As far as I can tell, there's no reason for those 4 lines to be there at this point in time, and removing them makes int and float locals to be emitted correctly inside loops

This example in clojure.org itself seems to assume that ints should work in loops http://clojure.org/reference/java_interop#primitives

Also from that same paragraph:

All Java primitive types are supported
let/loop-bound locals can be of primitive types, having the inferred, possibly primitive type of their init-form

I agree that it should be possible to let-bound primitives of other types - I'm really talking about what happens on recur.

What would you expect to happen for a fn recur target? I wouldn't expect primitives other than long or double to work there since they can't occur in the function signature.

Note that I haven't closed this ticket, still talking through this.

I've definitely run into cases where creating a primitive int loop/recur would be useful for tight interop loops given how common int-indexing is in Java (some of the alioth benchmarks in particular would benefit from this). I think the argument is far weaker for float though.

I don't think we need to worry about fn recur targets at all, given that the only possible primitive bindings there are either long or double, and int/floats would get widened anyway, but good point, the tests in a patch for this ticket need to be sure that case is indeed handled.

RE: floats – I recall people complaining about bad support for floats when using clojure for graphical processing.

Even if admittedly a weak argument, I'm always of the idea that we should strike to be as consistent as possible. I don't think anybody would expect let/loop locals to behave differently, or differences between primitive types (other than the documented limitation about long/double being the only working prim types for fn arguments/return vals)

I'll leave this one in here but I'm going to treat it as an enhancement to current behavior. I think there's a good chance that Rich will just say this is working as intended.

I don't think the example is a very good one though and would welcome a better example. The reservations regarding unchecked-inc-int do not seem correct or valid to me (as usage should be fine on longs and is not designed for perf reasons anyways). A good example would should usage of a Java api in a loop where int-indexing and int-math gives better performance.

I edited the title as the bug is in `loop`, not `recur`

Attached a patch that removes the useless widenings done by the compiler on loop bindings, here's a benchmark demonstrating the speedup gained when areducing over int-arrays:

Before patch:

Clojure 1.8.0
user=> (use 'criterium.core)
nil
user=> (let [xs (int-array (range 1e6))] (bench (areduce xs i ret 0 (+ ret (aget xs i)))))
Evaluation count : 64260 in 60 samples of 1071 calls.
             Execution time mean : 954.009929 µs
    Execution time std-deviation : 20.292809 µs
   Execution time lower quantile : 926.331747 µs ( 2.5%)
   Execution time upper quantile : 1.009189 ms (97.5%)
                   Overhead used : 1.840681 ns

Found 4 outliers in 60 samples (6.6667 %)
	low-severe	 4 (6.6667 %)
 Variance from outliers : 9.4244 % Variance is slightly inflated by outliers
nil

After patch:

Clojure 1.9.0-master-SNAPSHOT
user=> (use 'criterium.core)
nil
user=> (let [xs (int-array (range 1e6))] (bench (areduce xs i ret 0 (+ ret (aget xs i)))))
Evaluation count : 68640 in 60 samples of 1144 calls.
             Execution time mean : 870.462532 µs
    Execution time std-deviation : 13.100790 µs
   Execution time lower quantile : 852.357513 µs ( 2.5%)
   Execution time upper quantile : 896.531529 µs (97.5%)
                   Overhead used : 1.844045 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil

All

Simple patch attached. I did not find any existing tests for assoc-in but I could add them if wanted.

for the sake of symmetry with `assoc` I'd love to see this ticket fixed

Do you need the "if kvs" check?

Should have tests.

Sorry for the delay - I don't get notifications from this JIRA for some reason.

The patch now includes tests.

Both `if` checks are necessary as we have 3 possible outcomes there:
1/ No more kvs (we are finished)
2/ More kvs (we need to recur)
3/ A sequence of keys but no value (throw IAE)

Patch from Nicola Mometto

Attached the same patch with a more informative better commit message

I'd like to thank Christophe and Alex for their invaluable help in understanding what was happening, formulating the right hypothesis and then finding a fix.

I would also mention that even if non IBM rational environments where not affected by the bug to the point were CCW would not work, they were still affected. For instance the class for a one-liner function wrapping an interop call weighs 700bytes once the patch is applied, when it weighed 90kbytes with current 1.6 or 1.7.

In CCW for the initial problematic function, the -v2 patch produces exactly the same bytecode as if the referenced class does not load any namespace in its static initializers.
That is, the patch is valid. I will test it live in the IBM Rational environment ASAP.

I confirm the patch fixes the issue detected initially in the IBM Rational environment

I have absolutely no idea why, but if I apply this patch, and the patch for CLJ-1544 to master, and then try to build a war from this test project https://github.com/pdenhaan/extend-test I get a scary-looking traceback:

$ lein do clean, war!
Exception in thread "main" java.lang.NoSuchFieldError: __thunk__0__, compiling:(route.clj:1:1)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3606)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at extend_test.core.handler$loading__5301__auto____66.invoke(handler.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at extend_test.core.servlet$loading__5301__auto____7.invoke(servlet.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$compile$fn__5420.invoke(core.clj:5834)
	at clojure.core$compile.invoke(core.clj:5833)
	at user$eval5.invoke(form-init180441230737245034.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6776)
	at clojure.lang.Compiler.eval(Compiler.java:6765)
	at clojure.lang.Compiler.eval(Compiler.java:6766)
	at clojure.lang.Compiler.load(Compiler.java:7203)
	at clojure.lang.Compiler.loadFile(Compiler.java:7159)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$init_opt.invoke(main.clj:279)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.NoSuchFieldError: __thunk__0__
	at instaparse.core__init.load(Unknown Source)
	at instaparse.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at clout.core$loading__5301__auto____273.invoke(core.clj:1)
	at clout.core__init.load(Unknown Source)
	at clout.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:482)
	at compojure.core$loading__5301__auto____68.invoke(core.clj:1)
	at compojure.core__init.load(Unknown Source)
	at compojure.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at compojure.route$loading__5301__auto____1508.invoke(route.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	... 75 more
Subprocess failed

https://github.com/MichaelBlume/clojure/tree/no-field
https://github.com/MichaelBlume/extend-test/tree/no-field

mvn clean install in the one, lein ring uberwar in the other.

Michael, thanks for the report, I've tried investigating this a bit but the big amount of moving parts involved make it really hard to figure out why the combination of the two patches causes this issue.

A helpful minimal case would require no lein and no external dependencies, I'd appreciate some help in debugging this issue if anybody has time.

Ok, looks like the minimal case is

(ns foo (:require [instaparse.core]))

(ns bar (:require [foo]))

and then attempt to AOT-compile both foo and bar.

I don't yet know what's special about instaparse.core.

Well, not a minimal case, of course, but one without lein, at least.

ok, problem is instaparse's defclone macro, I've extracted it to a test repo

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

lein do clean, compile will get you a failure, but the repo has no dependencies so I'm sure there's a way to do that without lein.

Sorry for the barrage of questions, but these classloader bugs are subtle (and close to being solved I hope). Your report is immensely valuable, and yet it will help to be even more specific. There are a cluster of these bugs – and keeping them laser-focused is key.

The minimal case to which you refer is the NoSuchFieldError?
How are is this being invoked this without lein?
What are you calling to AOT? (compile 'bar) ?
What is the classpath? When you invoke originally, is ./target/classes empty?
Does the problem go away with CLJ-979-7 applied?

I have tried and failed to replicate without leiningen. When I just run

java -Dclojure.compile.path=target -cp src:../clojure/target/clojure-1.7.0-aot-SNAPSHOT.jar clojure.lang.Compile thunk-fail.first thunk-fail.second

everything works fine.

The NoSuchFieldError is related to the keyword lookup sites.

Replacing defclone's body with
`(do (:foo {})) is enough to trigger it, with the same ns structure.

I have updated the patch for CLJ-1544, now the combination of the new patch + the patch from this ticket should not cause any exception.

That said, a bug in this patch still exists since while the patch for CLJ-1544 had a bug, it was causing a perfectly valid (albeit hardly reproducible) compilation scenario so we should keep debugging this patch with the help of the bugged patch for CLJ-1544.

I guess the first thing to do is figure out what lein compile is doing differently than clojure.Compile

Also Ghadi is right, infact replacing the whole body of thunk-fail.core with (:foo {}) is enough.

It would seem like the issue is with AOT (re)compiling top-level keyword lookup sites, my guess is that for some reason this patch is preventing correct generation of the __init static initializer.

I still have absolutely no idea what lein compile is doing but I figured out the issue.
The updated patch binds (in eval) the appropriate vars only when already bounded.

Would it be worth using transients on the bindings map now?

Makes sense, updated the patch to use a transient map

Is there a test we can add that'll fail in the presence of the v2 patch? preferably independent of the CLJ-1544 patch? I can try to write one myself, but I don't have a lot of familiarity with the Clojure compiler internals.

I'll have to think about a way to reproduce that bug, it's not a simple scenario to reproduce.
It involves compiling a namespace from an evaluated context.

Hello, is there any chance left that this issue will make it to 1.7 ?

Wasn't planning on it - what's the impact for you?

The impact is that I need to use a patched version of Clojure for CCW.
While it's currently not that hard to follow clojure's main branch and regularly rebase on it or reapply the patch, it's still a waste of time.

I will check with Rich whether it can be screened for 1.7 before we get to RC.

same as v4 patch, but just has more diff context

the file mentioned in the patch field is not the right one IMHO

which one is?

I think you previous comment relates to clj-1620-v5.patch, but at the end of the description there's the following line:

Patch: 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v4.patch

Those patches are equivalent with respect to the change they introduce; they just differ in how much diff context they have.

Rich has ok'ed screening this one for 1.7 but I do not feel that I can mark it screened without understanding it much better than I do. The description, code, and cause information here is not sufficient for me to understand what the problem actually is or why the fix is the right one. The fix seems to address the symptom but I worry that it is just a symptom and that a better understanding of the actual cause would lead to a different or better fix.

The evolution of the patches was driven by bugs in CLJ-1544 (a patch which has been pulled out for being suspect for other reasons). Starting fresh, were those modifications necessary and correct?

Why does this set of vars need to push clean impls into the bindings? Why not some of the other vars (like those pushed in load())? The set chosen here seems to match that from the ReifyParser - why? Why should they only be pushed if they are bound (that is, why is "not bound" not the same as "bound but empty")? Are we affecting performance?

Popping all the way out, is the thing being done by CCW even a thing that should be doable? The description says "Compiling a function that references a non loaded (or uninitialized) class triggers its init static" - should this load even happen? Can we get an example that actually demonstrates what CCW was doing originally?

Alex, the question of "should what CCW is doing be doable" can be answered if you answer it on the given example, I think.

The question "should the initialization of the class occur when it could just be loaded" is a good one. Several reports have been made on the Clojure list about this problem, and I guess there is at least one CLJ issue about changing some more classForName into classForNameNonLoading here and there in Clojure.
For instance, it prevents referencing java classes which have code in their static initializers as soon as the code does some supposition about the runtime it is initialized in. This is a problem with Eclipse / SWT, this a problem with Cursive as I remember Colin mentioning a similar issue. And will probably is a problem that can appear each time one tries to AOT compile clojure code interoperating with java classes who happen to have, somewhere within static initializers triggered by the compilation (and this is transitive), assumptions that they are initialized in the proper target runtime environment.

What I don't know is if preventing the initialization to occur in the first place would be sufficient to get rid of the class of problems this bug and the proposed patch tried to solve. I do not claim to totally what is happening either (Christophe and Nicolas were of great help to analyze the issue and create the patch), but as I understand it, it's a kind of "Inception-the-movie-like" bug. Compiling a fn which triggers compiling another fn (here through the loading of clojure namespaces via a java initializer).

If preventing the initialization of class static methods when they are referenced (through interop calls - constructor, field, method, static field, static method-) is the last remaining bit that could cause such "compilation during compilation" scenario, then yes, protecting the compilation process like Nicolas tried to do may not be necessary, and just fixing the undesired loading may be enough.

I don't know the official stance on this ticket, but will add some notes.

Aaron, numerator and denominator are pretty clearly documented to work on Ratio types only.

It is pretty easy to write my-numerator and my-denominator that work exactly as you wish, checking for the type of arg and using numerator, denominator for Ratio types, and doing whatever you think is correct for other numeric types.

I'm aware that they are documented as such. Part of my point is that you can be working entirely with Ratio types and, via arithmetic operations between them, sometimes wind up with a non-Ratio number unexpectedly.

Also consider:

user=> (numerator 2/1)
ClassCastException java.lang.Long cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3238)

You're then left either implementing a try/catch correction or always checking the type before using numerator or denominator which is a loss in performance.

The patch I have in mind is creating a protocol, extended to Ratio, BigInt and Long which calls the appropriate method (Ratios) or returns either the given number or 1 (numerator/denominator) for the integral types. I expect this to maintain the current level of performance in the cases where it works and behave properly in the cases currently not handled.

I've definitely written the helper functions Andy describes on several occasions.

Related issue: https://stackoverflow.com/questions/25194809/how-to-convert-any-number-to-a-clojure-lang-ratio-type-in-clojure

A workaround to that is (numerator (clojure.lang.Numbers/toRatio (rationalize <put any type of number here>)))

I agree with the intent of the ticket here that these should work. I'm not sure about the protocol approach as that would be an open system and I'm not sure that's what we want. An alternative would be to just create Java methods on Numbers that took the appropriate types and let the JVM sort it out.

Any progress on this?

Sitting in the queue, waiting for love.

Can I give it love? If there's a direction that we can agree on, I'd be happy to create the patch.

I've screeened the patch that's here already?

The new versions of numerator and denominator after this patch was applied perform reflective calls. Is that expected?

Created ticket CLJ-2408 to make the question more prominent than a comment on a closed ticket.

Patch reverted

Maybe this is a separate issue, but `(.equals (vector-of :long 0 1 2) (range 3))` is also false (i.e. without calling seq on the return value of vector-of).

Same issue, and is fixed by the patch.

Excellent. Sorry, I had not read the new tests in the patch carefully enough to see that this is covered there. Good to know.

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

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

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

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

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

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

Incorporate changes from CLJ-1857 in clj-1360-2.patch.

Similer to CLJ-1232.

Patch updates for current master, retains attribution.

Add test and fix behaviour

Actually, although this passes all tests, it's not alright because it bypasses validation.

I think it passes for correctness this time but open to advice if this is not a good approach.

Clojure 1.9.0-alpha17
test.check 0.10.0-alpha2

If we are going to handle this case, can we also include the name of the offending symbol in the error message?

The sym-or-f arg is resolved to nil before exercise-fn is invoked, so we have no symbol to report by that point.