Clojure 1.10.0-alpha6
clojure.spec.alpha 0.2.168

n/a

Hi Keyhan, have you signed the Clojure Contributor Agreement? https://clojure.org/community/contributing. If not, please do so.

Another, maybe better, solution here is (<= 0 port 65535) - that reads more closely to the check.

Yes, just earlier today.

I agree that looks better. Current patch is port-65535-obo-v2.patch.

linux

Output from a REPL session with the "-1" patches: https://gist.github.com/stuarthalloway/d3055d1975b464e8949b854e77a746b1

re "many Clojure tests that parse exception strings need to change":

Is it desirable to decouple the error message (as printed to the user) from the error tag/type/kind (for tooling, tests, etc), and make the latter a part of the contract?

I get the eliding map values from the output is meant to hide the ginormous Spec explanations. But printing only the keys doesn't have much value (pun intended). Is there possibly another solution? I can see two suboptimal ones:

1) Dissoc only the :clojure.spec.alpha/ keywords from the ex-data, Spec being the main offender.
2) Print ex-data with small print-length and print-depth (not sure whether that achieves anything, have to analyze different Spec errors to see if it does).

Thanks for the suggestions!

jcr: That might be desirable, but it is an expansion of scope and is not necessary to solve this problem.

Alexander: This is not just about spec errors, but all ex-data. The data is already present and accessible, and printing the keys gives the user a hint where to look.

I have dug into the test failures more and there are really two things here...

1) the top level exception messages have changed (and are generally worse for the particular case of a CompilerException with nested init exception, which is not in the example set). I think this bears revisiting for those cases where you might be in a "Java calling Clojure" environment.

2) the message printed to the repl has generally improved, however most of the failing tests are checking exception message as a proxy for what the user sees in a particular error situation via thrown-with-msg?. It would be helpful if "what is printed" could be more easily extracted and if the tests were actually checking that, rather than checking the exception message - I think this better reflects the tests' intent.

I'm working on an updated patch.

clj-2373-2.patch makes fewer changes in the CompilerException message (tests all continue to pass) but does more in the printing of exception chains in the repl, ignoring the computed message of the wrapper CompilerException completely and focusing more attention on the initial cause (but adding back in the source location info in the CompilerException wrapper if it's available).

user=> (/ 1 0)
ArithmeticException Divide by zero clojure.lang.Numbers.divide (Numbers.java:163)

user=> (throw (ex-info "oops" {:a 1 :b "foo"}))
ExceptionInfo oops clojure.core/ex-info (core.clj:4754)
ex-data keys: [:a :b]

user=> (let [a])
ExceptionInfo Call to clojure.core/let did not conform to spec., compiling:(3:1) clojure.core/ex-info (core.clj:4754)
In: [0] val: () fails spec: :clojure.core.specs.alpha/bindings at: [:args :bindings :init-expr] predicate: any?,  Insufficient input

Dumped some further design work at https://dev.clojure.org/display/design/Exception+handling+update

Still a WIP, take nothing here as final.

Pushed -3 patch for current state (does not address test failures yet).

A vote for principles of brevity + data-to-the-edge. At the point of throw, code has no idea how many GB of stuff it's including in ex-data and no basis on which to prune it.

All

Attached a patch for this.

For defrecord, I check which style is used for defining methods, and transform into the original style if the new style is used. For the check I do what I believe defn does, which is (vector? (first fdecl)).

For extend-*, I skip the checking, and just transform everything into the same format.

Tests included for both.

All tests pass.

What the proposal?

This hit me too.

I workaround this using extend for the protocols that have multi arity methods
i.e. for the above example

(extend Object
  MyProtocol
  {:mymethod (fn ([^Object this arg] :one-arg)
                 ([^Object this arg optional-arg] :two-args))})

For the moment I’ll say sure. I think this particular example is a good one.

I’m not sure that all the specs here are as obvious as you expect, or at least that’s been my experience elsewhere.

I’d expect these specs to be in clojure.set.specs namespace btw.

Attached CLJ-2287-add-clojure-set-specs-v1.patch dated 2017-Dec-13. These are my first Clojure specs ever, so feel free to go crazy on suggesting improvements.

I used the namespace clojure.set.specs.alpha rather than clojure.set.specs as you suggested, but I can easily change that if you really did want it without the .alpha

If there is some way I can exercise the :args specs with a large suite of tests, let me know how and I can try it.

And clearly I am putting in at least for :args and :ret pretty obvious choices of specs that I believe are correct, given issues raised against those functions in the past. I don't see how they could be any more subtle than that for these functions, but let me know if I'm missing anything, e.g. the Clojure core team has decided to support non-set arguments for these functions.

I agree that some of the other functions in the clojure.set namespace might be a bit trickier to spec the args of.

I looked through all of Clojure and contrib projects for occurrences of clojure.set/union, difference, intersection, superset?, and subset?, and there is only one such call that I can quickly determine does not pass sets to them. It is the one in clojure.data/diff for which there is a patch that can 'correct' this (assuming it is considered a bug) on this ticket: https://dev.clojure.org/jira/browse/CLJ-1087

Many such calls I could quickly determine always passed sets (e.g. because the arguments were wrapped in (set ...) calls). On a small fraction of them, I gave up trying to figure it out after a minute, since determining the answer fully from code inspection would require going back up the call tree a ways.

If you want an off-the-shelf compatible replacement for clojure.set functions that are identical in behavior, except they perform run-time type checks of the arguments you provide to them, and throw an exception if they have the wrong types (e.g. not sets for union, intersection, difference, subset?, and superset?), consider using the fungible library: https://github.com/jafingerhut/funjible

Android ART runtime

After using this a little more, I've found that moving this outside the try block breaks nREPL.

Looking at the bytecode, the monitorenter for the locking in clojure.tools.nrepl.middleware.session/session-out and in a few other places ends up in an entirely different method definition and we now get a JVM IllegalMonitorStateException as well as an ART verification error for this function.

Adam, I cannot comment on whether your patch is of interest or not, but it is true that no patch will be committed to Clojure if the author has not signed a Contributor Agreement, which can now be done on-line at http://clojure.org/contributing

Uploaded a new patch (and signed the contributor agreement). This passes both the JVM and ART bytecode verification, The extra try/catch around the monitor exit is optional (verification passes with or without it) but where the java version retries monitor-exit indefinitely and shows the deadlock at the right time, without catching errors in the monitor-exit an undetermined monitor-enter in the future might fail, not showing up the actual bug.

It's not very pretty, but without finer grained control of the generated bytecode, this is the best I could do.

Have just tested with Lollipop, and this patch might no longer be sufficient.

Getting in touch with the ART guys to see if they can shed a little more light and verify whether it will work on the current master branch of AOSP

Bug filed with AOSP project, hopefully they can shed some light on whether it is our problem and if so how we can fix it.

https://code.google.com/p/android/issues/detail?id=80823

I have uploaded an alternative implementation of the locking macro (0001-CLJ-1472-Locking-macro-without-explicit-monitor-ente.patch) which cheats a little - the synchronized block is actually implemented in Java and so guarantees compatibility. This is at the cost of a little extra indirection and the naming/location could probably be better.

But it does fix the bug and work on all versions of android, android + art and the jvm. Would this approach be acceptable?

I have yet to see any evidence that the bytecode clojure is generating in some way violates the jvm spec, so I suspect the issue is clojure requires a jvm to run, and android doesn't provide a jvm, just something that looks like one if you don't tread outside the beaten path.

given the structured locking verbiage in https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-2.html#jvms-2.11.10, `(locking nil)` may generate bytecode whose runtime behavior violates structured locking. the first patch on this issue can cause the compiler to emit monitorenter/exit instructions in different methods, which definitely violates structured locking

Yes, the first patch was definitely wrong, I left it for some context to the conversation, but it's probably best to just remove it for clarity.

For anyone following this conversation who doesn't want to decompile and observe the bytecode, here's a gist with the difference between a java synchronized block and clojure locking https://gist.github.com/AdamClements/2ae6c4919964b71eb470

I'm finding it hard to work out where the deviation from the spec occurs too, though I can see the difference with the Java version, if anything, the Clojure version looks closer to what's described in the spec than the Java one!

If someone with more knowledge than me on the subject could engage on the AOSP bug https://code.google.com/p/android/issues/detail?id=80823 then perhaps we could settle this as an android bug which is too focussed on the java implementation rather than the JVM spec, or perhaps they'll find something that's wrong with the Clojure implementation. I have uploaded the original clojure behaviour and asked them for some more explanation on why it is failing.

The response from the ART guys about what they think we're violating is:

The section on "Structured locking" contains the following:

"[...] implementations [...] are permitted but not required to enforce
both of the following two rules guaranteeing structured locking. [...]"

ART currently enforces both rules at verification time, including

"At no point during a method invocation may the number of monitor exits
performed by T on M since the method invocation exceed the number of
monitor entries performed by T on M since the method invocation."

If for example instruction https://gist.github.com/AdamClements/2ae6c4919964b71eb470#file-test_locks-class-L24 or the monitor-enter itself on the next line were to fail, couldn't it could end up in the finally clause and attempt to release the lock even though it has never been captured?

I think this violates the structured locking rules in the jvm spec you linked to.

an interesting question about structured locking, is does the specification refer to the static bytecode or the runtime behavior of the bytecode. given the bytecode linked(https://gist.github.com/AdamClements/2ae6c4919964b71eb470#file-test_locks-class-L24), the static bytecode has the same number of entries and exits, but the dynamic behavior may be different. I wonder which one the art guys claim to be enforcing at verification time (it seems like it would have to be the static bytecode, not the dynamic properties, but then they shouldn't be failing to verify this). looking at the google code issue, the comment https://code.google.com/p/android/issues/detail?id=80823#c6 was made by the same dev as https://code.google.com/p/android/issues/detail?id=80823#c3, so I sort of suspect there is some miscommunication going on. It is not clear in what context the dev is replying in, since in the previous comment you mention splitting monitor-enter and exit across methods. I think things would be much clearer if all patches, specialized clojure android builds, etc, were gotten rid of, then with a vanilla clojure jar you get a javap dump of what fails to verify, then just take that over to the android issue tracker and ask "hey, this fails to verify, why?"

Yeah, I shouldn't have confused it with the patched versions. The gist and the currently uploaded version use the vanilla clojure version of the locking macro now though.

I think the issue comes from the exception table and the instructions that covers. If line 24 can throw for example, you would end up at runtime with a monitor-exit, having never encountered a monitor-enter.

According to Marcus Lagergren JRockit and Hotspot both account for lock releases being in a different method... Perhaps Android has a different (wrong) interpretation?

I added a new clj-1472.patch that fixes a few minor details I didn't like about the prior patch. However, it is still essentially the same change so I have retained the original author attribution.

alex did you upload the right patch?

Ah, no messed that up. Will fix.

Alex, it's probably worth making that a `^:once fn*`

From Android:

Android doesn't run Java bytecode, it runs Dex bytecode. The dexdump output of what your class translates to is interesting.

Neither is the JVMS interesting. Android isn't a Java Virtual Machine. We follow the JLS, but not the JVMS (how could we, when we don't run Java bytecode). As such, all appeals against it are irrelevant. We try to be compatible to the spirit of the JVMS wrt/ Dex bytecode, but if your source isn't Java, there are no guarantees.

Now, the verifiers were (and probably still are) broken, even against our (pretty bad) spec, and regrettably we aren't very consistent. In Marshmallow, for example, a lot of the code we can't correctly verify wrt/ structured locking is rejected as a VerifyError, which is not in the spirit of the JVMS. In the next release, this will be relaxed, however, and postponed to an actual check while running the code.

Regrettably there isn't anything we can do about old releases, you'll have to work around any issues. I'll try to take a look at your class when I find the time.

Sounds like making a workaround in Clojure is the least of all evils.

Added -2 patch with ^:once.

Our current belief is that Android is at fault here, so backlogging this for now.

GraalVM/native-image is also complaining about monitorenter/exit balancing. Fixed it by mimicing what javac does: https://github.com/ghadishayban/clojure/commit/8acb995853761bc48b62190fe7005b70da692510

Ghadi, if that's a viable fix, I'm interested in a patch for that.

If someone could assist me with Android testing infra, I'd be game.

In `javac`'s emission of synchronized, it installs a catch handler with the exception type "any". The linked commit catches Exception. If desired, we can emit the `any` handler (I don't know what that means yet exactly – Throwable + anything else in the future?) by calling GeneratorAdapter.visitTryCatch with `null` as the target class.

Have you tried applying the existing clj-1472-2.patch to see if that fixes GraalVM? I think we'd originally reached a place where people were happy with the attached patch (correct me if I'm wrong Alex) but it was decided that the JVM implementation is king so the problem was with Android for not conforming to that rather than with Clojure. It could be that decision is up for reconsideration if the same problem has reared its head elsewhere (I'd still like to see this patched myself).

This was an age ago now, but I remember trying all different combinations of clojure try/catch/finally/throw I could think of and not being able to get the emitted bytecode to conform to the spec for synchronised without altering clojure's code generation which was far too deep down to be practical - hence the above patch which relies on javac to generate the bytecode around the synchronisation instead of using monitor-enter and monitor-exit. I'd be concerned with your linked implementation that it might still generate wrong bytecode in some situations much like the existing implementation does even though it looks perfectly fine and apparently correct at the Clojure code level.

I hadn't tried it but I'm sure clj-1472-2.patch would work from a feasability perspective. It has a perf downside: it can cause "profiling pollution" from the JVM's perspective and confuse lock optimization. (Most locks are uncontended)

I'd rather fix the emission to be more like javac, iff that works for Android.

Ok, attached `assoc.diff`, which unrolls this to a single level more than the current code (so supporting two key/value pairs without recursion). The code's going to get pretty complex in the case with more than the unrolled number of keys if we continue on this way, so I'm unsure if this is a good approach, but the performance benefits seem very compelling.

Since the unroll comes out kind of hairy, why not have a macro write it for us?

Patch v2 includes assoc!

I benchmarked conj with similar unrolling, across a relatively wide range of datatypes from core (lists, sets, vectors, each one empty and then again with 20 elements):

Benchmarks were run on the same machine as before. There's a less clear benefit here, except for lists, where the overhead of creating seqs and recursing seems to be clearly dominating the cost of actually doing the conj (which makes sense - conj on any element list should be a very cheap operation). Raw benchmark output is here: https://gist.github.com/tcrayford/51a3cd24b8b0a8b7fd74

Michael Blume: I like those patches! They read far nicer to me than my original patch. Did you check if any of those macro generated methods blew Hotspot's hot code inlining limit? (it's 235 bytecodes). That'd be my only worry with using macros here - it's easy to generate code that defeats the inliner.

Thanks! This is new for me, so I might be doing the wrong thing, but I just ran nodisassemble over both definitions and the "instruction numbers" next to each line go up to 219 for the varargs arity assoc and up to 251 for assoc!, so, assuming I'm looking at the right thing, maybe that one needs to have an arity taken off? If I remove the highest arity I get 232 for varargs which is just under the line.

I guess alternatively we could call assoc! instead of assoc!* in the varargs arity, which removes a lot of code – in that case it's 176 for varargs and 149 for six pairs.

Gah, I forgot to include coll in the varargs call to assoc!

which reminds me that this patch needs tests.

OK, this has some fixes I made after examining the disassembled output. There's a change to the assoc!* macro to be sure it type-hints correctly – I'm honestly not sure why it didn't type-hint properly before, but it does now. Also, I changed the call to assoc! rolling up the first six entries at the top of the varargs version from a macro call to a function call so it'd fit within the 251 inlineable bytecodes. (This, again, is assuming I'm reading the output correctly).

Michael: Wanna push a branch with these patches to clojars or something? Then I can rerun the benchmarks with the exact code in the patches.

Hmm, not sure I know how to do that – here's a branch on github though https://github.com/MichaelBlume/clojure/tree/unroll-assoc

v5 marks the helper macros private.

Michael: was that branch just based off clojure/clojure master? I tried running benchmarks off it, but ran into undefined var errors when building this code (which doesn't happen with alpha5):

(Retrieving com/yellerapp/clojure-unrolled-assoc/1.7.0-unrollassoc-SNAPSHOT/clojure-unrolled-assoc-1.7.0-unrollassoc-20150213.092242-1.pom from clojars)
(Retrieving com/yellerapp/clojure-unrolled-assoc/1.7.0-unrollassoc-SNAPSHOT/clojure-unrolled-assoc-1.7.0-unrollassoc-20150213.092242-1.jar from clojars)
(Retrieving org/clojure/clojure/1.3.0/clojure-1.3.0.jar from central)
Exception in thread "main" java.lang.RuntimeException: Unable to resolve symbol: bench in this context, compiling:(bench_assoc_unrolling.clj:5)
at clojure.lang.Compiler.analyze(Compiler.java:6235)
at clojure.lang.Compiler.analyze(Compiler.java:6177)
at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3452)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6411)
at clojure.lang.Compiler.analyze(Compiler.java:6216)
at clojure.lang.Compiler.analyze(Compiler.java:6177)
at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5572)
at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5008)

Ok, how are you building? Why the strange clojure group?

The existing version of assoc does runtime checking that an even number of varargs are passed in, but assoc! does not. Do we want to preserve this behavior or do checks in both?

Also, I'm curious how relevant inlining is here – does HotSpot inlining actually work with Var invocation when there's a getRootBinding step in the way?

Yes, inlining works through var invocation.

Michael,

That group is just an uploaded version of clojure master with your patches applied, built in just the same way as before (you should be able to check out the repo and replicate).

The patch CLJ-1656-v5.patch doesn't seem to do anything with the old version of assoc (in core.clj around line 179)?

The new one needs to have the arglists and other stuff like that. I'm not sure about the macro/private vars in there either. Did you try leveraging RT.assocN() with a vector?

Are there existing tests in the test suite for assoc with N pairs?

The dependencies in clojure.core were such that assoc needed to be defined well before syntax-quoting, so I just let it be defined twice, once slower, once faster. I'll put up a patch with arglists. Does it need an arglist for every new arity, or are the existing arglists enough? (I'm afraid I'm not 100% solid on what the arglists metadata does) There is an annoying lack of existing tests of assoc. I have a generative test in my tree because that seemed more fun than writing cases for all the different arities. I can post it if it seems useful, it might be overkill though.

Here's the test patch I mentioned, it's even more overkill than I remembered

There, code and test.

This also checks that assoc! is passed an even number of kvs in the varargs case, which is the behavior of assoc. The test verifies that both assoc and assoc! throw for odd arg count.

The existing arglist is fine - it just overrides the generated one for doc purposes.

Did you try any of the RT.assocN() stuff?

I guess another question I have is whether people actually do this enough that it matters?

Updated patch to apply to master

This still needs assessment on frequency of different counts. Based on some very quick checking, passing two kvs is common enough to matter. Passing three is far less common and so on. But I'd love to see some rough idea of frequency from running some stuff. I think the current unrolling is too much (2-3 kvs is probably sufficient).

approach in the patch

• load clojure.instant unconditionally, as well as the #inst reader
• wrap only the sql.Timestamp print-method extensions with a conditional load
• remove when-class (now unused, it will not work for wrapping the defmethods because the java.sql.Timestamp class constants need to be delayed)

Besides AutoCloseable, the rest of those classes are rarely used. I'd rather import them explicitly over polluting the ns imports tbh. (Interfaces annotated with FunctionalInterface, an annotation type, are common; using it explicitly isn't)

There is a minor startup time cost to additional classloading, for classes not already loaded by the java.base module. I've experimented with dropping a bunch of rarely used default imports (a breaking change, no doubt).

I think we should uphold what has been true and stated in the past - stuff in java.lang is auto-imported. I do not think autoimporting 6 classes will affect startup time.

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.)

Windows, Java 1.8

I believe what's happening here is that in the first case, it gets typed as a PersistentVector (concrete data structure read by the reader). PV implements Collection and so requires no reflection.

In the second case, the vector requires evaluation and its type is IPersistentVector (the interface). IPV does not extend Collection (its independent of that interface) and so it can't find the indexOf method.

Stepping back from both, we're doing Java interop on an object that we are expecting to be a Collection. I'm somewhat on the fence about whether we should expect ids to carry this info when performing interop on it here. Depends how much you think that the abstract Clojure vector collection is a Java collection (vs the implementation of one).

Doesn't seem easily "fixable" to me in ways that are clean.

This code is not valid because x is an invalid alias (not bound to anything).

This works for example:

user=> (alias 'x 'clojure.string)
nil
user=> (comment ::x/y)
nil

comment still reads the code, so code that is unable to be read is still invalid.

Note that CLJ-2030 would make this code work (by auto-creating x as a new namespace).

I am totally for this. I agree that comment isn't a valid substitution. comment is useless for a lot of things.

Nice - but are you sure just doing nothing on an unexpected EOF is a good idea?
I guess it usually means I've forgotten the close tag somewhere, so I'd rather be informed about it by an exception. Omitting the |# on purpose just to save two characters seems dirty to me.

What about #_(...)?

I know that the parens have to be balanced, but other than that, doesn't it do everything else?

No.

user=> #_(TODO: Hi there!)
java.lang.Exception: Invalid token: TODO:

And then, as you mentioned, we have the paren balance problem. The idea is to have something completely ignores what is inside of it.

Good point, Benjamin. I just assumed most languages didn't need comments to close nicely, but having tried it out I can't find a single one that would let me get away with an unclosed multi-line comment.

Revised patch attached, and using git format-patch now that I know it exists.

You can use #_ with string literals already:

#_"TODO: Hi there!"

The only problem we have is that we don't have raw strings. I don't think that we need another litrals for comments.

And I don't see any use for multiline comments. In Java etc. you use them to uncomment parts of your program (we have #_ (...) for that) or to document classes & methods (we have docstrings which are superior). If you need multiline comments to explain some part of your code, then you should do something about the code (or use #_ "..." if you are lazy).

Except with #_"" you can't just paste something in there because it has to be escaped properly if it has strings and such embedded in it.

Sure, we can work around these limitations somewhat. We can pile hacks on hacks to get half-working multiline comments or just add this trivial patch and have real multiline comments like every other language in existence. Just sayin'.

Multi-line comments aren't for just explaining bad code. #_ isn't designed for this and thus is not a solution to this, and marking every line of a prewritten text with ; and/or removing or editing it later is just tedious.

Curiosity: wouldn't this be useful for literate programming tools?

Rich: if you are interested in this assign to me and I will screen.

Don't think that this is a pressing issue, because every editor that has support for Clojure (or even just Scheme/CL) has a comment-region feature.

I would support adding a reader macro for raw strings, because that is an issue (almost every ns docstring is escaped), and if we had those, then multiline comments would be subsumed by them:

#_
#[[ ;; lua-like proposal for raw strings

]]

If its a pressing issue or not isn't really the question is it?

To me its a straightforward patch which adds functionality currently not found in Clojure and which I personally would label a welcomed addition.

Sure it adds a feature, but it also adds syntax and complexity to the language. If the feature it adds doesn't solve real problems of sufficient importance, it may be a net loss for the language.

For temporarily commenting out code, #_ and (comment ...) work fine. For hunks of documentation text, ; or ;; work nicely, especially if you have an ultra-advanced editor like vim to handle your text reformatting. Is there another use case?

I'm with Chouser on this. Additionally, I would add that the description of #_ is "The form following #_ is completely skipped by the reader." However, trying something like:

    (defn gimmie-pi []
      #_( TODO: validate the accuracy
                of the return value. )
      3)

currently throws an exception. Maybe the form should really be skipped by the reader and allow free form text? I don't know if this is the right answer, but I would feel more comfortable with that than a new reader macro.

I don't think that's possible, Fogus. If it's going to skip "a form", then it has to understand the syntax therein well enough to determine which closing paren matches up.

> which closing paren matches up

I'm sorry, I was not clear that I didn't expect my idea to provide the entire functionality of the proposed #| ... |#.

I don't see the utility in providing a whole new reader macro to allow mismatched embedded parentheses. I can kinda see the value in allowing things like `TODO:`, although I would probably just use #_() or comment and just make sure that my embedded comment was not malformed and ; otherwise.

I'm opposed. I don't think the benefit/complexity ratio (especially for tools) is high enough. We've done without for quite a while and no one's caught fire.

Closed per request from Alan Malloy.

$java -jar clojure-1.5.0-RC2.jar

$java -version
java version "1.6.0_37"
Java(TM) SE Runtime Environment (build 1.6.0_37-b06-434-10M3909)
Java HotSpot(TM) 64-Bit Server VM (build 20.12-b01-434, mixed mode)
Mac OS X:
System Version: Mac OS X 10.6.8 (10K549)
Kernel Version: Darwin 10.8.0

this is an artifact of how streams and repls work.

when you type (defn 45fn [] nil) and hit enter, the inputstream flushes and "(defn 45fn [] nil)" is made available to the reader, the reader reads up to 45fn, throws an error back to the main repl loop, which prints out the error, then calls read, which still has the unread parts available to it "[] nil)"

changing this behavior would require significant changes to clojure's repl.

checkout https://github.com/trptcolin/reply instead

Why does reader continue to read after encountering an exception? Shouldn't it immediately stop?

If someone could point me to the relevant code, that'd be helpful.

To "immediately stop" in a REPL, do you mean stop reading anything more in the REPL session at all? If you did that, it would just 'freeze up' the REPL session. I don't think anyone wants that behavior.

The reader doesn't continue to read after encountering an exception. It raises the exception. The L (loop) part of the REPL goes back to read again, starting at the last part not read yet, which in the example is "[] nil )", as Kevin Downey explained in his comment.

Perhaps you could imagine a different behavior something like: if the reader raises an exception, then before the REPL goes back to read again, it should discard the rest of the input line it was on if an exception was raised. I don't know whether such a thing is a change desired by the Clojure core team in the standard REPL. You can write your own REPLs that do that, of course.

See the function 'repl' in the clojure.main namespace. At a REPL, you can see it by evaluating: (source clojure.main/repl)

Addendum: Stuart Halloway's talk on REPL-Driven Development has some notes on customizing the built-in REPL, or writing your own. See: https://github.com/matthiasn/talk-transcripts/blob/master/Halloway_Stuart/REPLDrivenDevelopment.md

if the reader raises an exception, then before the REPL goes back to read again, it should discard the rest of the input line it was on if an exception was raised.

That's certainly what I'd expect--but it shouldn't discard the rest of the input line, rather the entirety of the remaining input.

The way the REPL currently works means it does not set *e to the first exception that caused it to fail. The only indication the user has of the "real" error is a printed message (the result of (catch e)). They (or rather their tooling) aren't able to analyze the right error.

What's more, the current behavior could lead to some surprising (and potentially destructive) results.

For example,

=> (do {:foo} (println "this shouldn't happen") 
RuntimeException Map literal must contain an even number of forms  clojure.lang.Util.runtimeException (Util.java:221)
this shouldn't happen
nil

This example is trivial, but imagine that the println was some destructive operation dependent on the validity of the preceding form.

The following solves the problem:

(defn flush-input
  "Flush input reader, default *in*."
  ([]
   (flush-input *in*))
  ([in]
   (loop []
     (when (.ready in)
       (.read in)
       (recur)))))

(defn repl-caught
  "Default :caught hook for repl"
  [e]
  (let [ex (repl-exception e)
        tr (.getStackTrace ex)
        el (when-not (zero? (count tr)) (aget tr 0))]
    (flush-input)    ;; new
    (binding [*out* *err*]
      (println (str (-> ex class .getSimpleName)
                    " " (.getMessage ex) " "
                    (when-not (instance? clojure.lang.Compiler$CompilerException ex)
                      (str " " (if el (stack-element-str el) "[trace missing]"))))))))

R.e. the original issue:

=>  (defn 45fn [] nil)
NumberFormatException Invalid number: 45fn  clojure.lang.LispReader.readNumber (LispReader.java:332)

=> *e

#error {
 :cause "Invalid number: 45fn"
 :via
...

A caution here: "Discard the rest of the input" might be a nondeterministic approach in some scenarios. I haven't tested this, but suppose I copy and paste a block of 20 lines of code into the REPL, and the reader raises an exception somewhere during the first line. "The rest of the input" at that instant might be the rest of the first line, or half of the 20 lines, or all of it. It may depend upon how data was broken up across TCP packets in a socket REPL, or the timing of how your terminal decides to present the characters to the Clojure process.

Don't think that'd be a worse outcome than currently. In both cases the remainder of the bad input is evaluated. And I'm not sure how much of a problem this would be--most data entered into the REPL is a few lines at most.

A difference between the way it is now, and your proposal, if I am correct in my guess that the 'discard the rest of the input' approach is nondeterminstic, is that non-determinism that you are introducing. I could copy and paste the same 20 lines in the current REPL, and always get the same exceptions (given the same starting state, if that mattered).

I could copy and paste the same 20 lines in your modified REPL, and sometimes get different behavior.

If someone filed a bug for that non-determinism, and you were responsible for responding to it, how would you respond?

I'd say fix the code you're sending to the REPL, b/c an exception has been raised. Then enter it into the REPL again after you've fixed it. Note that flushing the input does not occur unless an exception is raised.

Sure, but you can fix the code you're sending to the REPL, because an exception has been raised, with the code as it is today, too.

The flushing change would mean - without that change, reader exceptions result in a predictable point in the read stream where reading starts again. With the flushing change, reader exceptions result in an unpredictable point in the read stream where reading starts again.

you can fix the code you're sending to the REPL, because an exception has been raised, with the code as it is today, too.

The point is that it's harder to do so because the final error reported is not the first error from the read failure. Which error message would you rather see for OP's problem: NumberFormatException Invalid number: 45fn or RuntimeException Unmatched delimiter: )? And yes, the Invalid number ... exception will be printed by the REPL, but it won't be set as *e, which is the reified error available to tooling.

I don't know how big a problem the incomplete flushing you mention is. I'd surmise that for most input, it's not a problem. It hasn't been a problem for my locally modified version of Clojure. I know nothing about the socket REPL, so I've no idea if it'd be more common there.

To completely solve this problem the REPL would need to have some idea of what a "complete" input is--i.e. everything entered before the user hit RET. If someone has any pointers on how that could be done, I'd appreciate it. Otherwise I'd say we shouldn't rule out a 99% solution over a 1% edge case.

Would be nice to include the bad condition in the error (maybe via ex-info?) and also have tests.

New patch includes tests. Unfortunately, can't call ex-info directly due to bootstrapping concerns. Instead, just calls ExceptionInfo constructor directly.

Bug in the reporting: {:post pre} should be {:post post}.

Test should be improved as it could have caught that.

Good catch with the pre/post copy/paste screw up. Didn't enhance the test though, since that would involve creating an ex-info friendly variant of fails-with-cause

:pre and :post don't require vectors, just collections

Eastwood 0.2.2, released on Nov 15 2015, will warn about several kinds of incorrect pre and postconditions. See https://github.com/jonase/eastwood#wrong-pre-post

The Eastwood documentation may be misleading right now, in that it says that :pre and :post should be vectors, which is at odds with Rich's comment of Oct 9 2015. Corrections to Eastwood's documentation here are welcome. I guess Rich's intent is that :pre and :post could be vectors, lists, or sets? Would a map ever make sense there?

Hello any news ?

There's feedback above from Rich that has never been addressed here. I took a stab at an update, but relaxing the constraint to just "collections of expressions" is kind of interesting. Once you do that, then the first example in the ticket here {:pre (pos? x)} is actually not invalid, it's a collection of two things: pos? and x. pos? evaluates to true (it's a function) and x evaluates to true for any number.

I'm not sure what the way forward is on that. It's perfectly valid and even potentially useful to refer to an incoming arg as a precondition, which would just be a symbol:

( (fn [v] {:pre [v]} v) nil)  ;; precondition should fail
( (fn [v] {:pre (v)} v) nil)  ;; equally valid

Maybe you could check for whether the elements of pre or post are either lists (expressions) or symbols that are arg bindings? Anything else is vacuously true and probably a bug. Not sure.

Eastwood still gives warnings about code like the following:

(defn foo [x]
  {:pre [pos? x]}
  (inc x))

Here is sample output:

src/filename.clj:5:22: wrong-pre-post: Precondition found that is probably always logical true or always logical false.  Should be changed to function call?  pos?

It also still warns if the value of :pre or :post is not a vector, despite Rich's comment, so it is overly strict in that sense, but maybe not so bad to be overly strict there.

Can you add a Proposed line to the description explaining the change?

I'm not sure I get the problem/solution here.

sure, done

Looks like this breaks the serialized objects in the (admittedly fragile) clojure.test-clojure.java-interop/test-proxy-non-serializable (see CLJ-2204, CLJ-2330).

As discussed in #clojure-dev, attached patch that disables the brittle tests and restores the determinism of `proxy-name`.

Patch updated to address first screening comment. I didn't actually find any case of the second one? Give me a pointer.

I suspect that this will not matter much for peak performance. Given the attached picture of clojure.instant loading, this patch may shave a bit off startup time.

(picture made with Bytestacks, profile recorded a JDK11 debug build from today)

org.clojure/spec.alpha "0.1.143"

I came across another case of extra nesting, when repeating one or more sequences with an optional element at the beginning or end, where that element's predicate also matches the element at the other end:

user=> (s/conform (s/+ (s/cat :k any? :v (s/? any?))) [:a 1 :b 2])
[{:k :a, :v 1} [{:k :b, :v 2}]]

where I expected

[{:k :a, :v 1} {:k :b, :v 2}]

The following give expected results:

user=> (s/conform (s/+ (s/cat :k any? :v (s/? any?))) [:a 1 :b])
[{:k :a, :v 1} {:k :b}]
user=> (s/conform (s/+ (s/cat :k keyword? :v (s/? int?))) [:a 1 :b 2])
[{:k :a, :v 1} {:k :b, :v 2}]
user=> (s/conform (s/* (s/cat :k any? :v (s/? any?))) [:a 1 :b 2])
[{:k :a, :v 1} {:k :b, :v 2}]

Phil, I think your example is a different issue and you should file a new jira for that.

Well, maybe I take that back, they may be related.

I just ran in to this trying to make sense of some defn forms. Here's an example:

user=> (s/unform :clojure.core.specs/defn-args (s/conform :clojure.core.specs/defn-args '(f [& xs])))
(f ((& xs)))

This seems to be all that is needed.

The problem is actually more universal than (? (cat ...)). s/unform of a s/? with any regex child op will introduce an extra level of nesting. When the child is a regex, we are consuming the same "level" of sequence so unform should not introduce an extra level. However in other cases (non-regex ops), we should still possibly produce a nested collection.

The previous patch was too aggressive: it unwrapped all sub-unforms of s/?. This patch CLJ-2003-corrected.patch only unwraps when the sub-op is a regex.

Unfortunately it is impossible to distinguish between a desired-but-optional nil and a non-match from s/?. Specifically, the following tests now hold:

(testing "s/? matching nil"
  (is (nil? (s/conform (s/? nil?) [nil])))
  (is (nil? (s/conform (s/? nil?) [])))
  (is (nil? (s/conform (s/? nil?) nil)))
  (is (= (s/unform (s/? nil?) nil) [])))

(I did not add these tests to the patch because I was unsure if they should be part of the contract of unform. However, they are pretty big gotchas.)

I also added tests for every possible subop of s/?, except ::s/accept, which I could not think of a test case for. (I'm not sure ::s/accept is actually reachable inside s/op-unform?)

Thanks for working on this - I will take a look when I get a chance.

I had to amend my patch slightly (same name): one of the test cases wasn't testing the correct thing.

Patch was no longer applying cleanly to master because of tests added by other commits. Patch rebased to master

More odd side effects.

ti.repl-init=> (clojure.set/intersection #{:foo} {:foo 1})
#{:foo}
ti.repl-init=> (clojure.set/intersection #{:foo} {})
{}
ti.repl-init=> (clojure.set/intersection #{:foo} [:foo])
#{}
ti.repl-init=> (clojure.set/intersection [:foo] [:foo])

ClassCastException clojure.lang.PersistentVector cannot be cast to clojure.lang.IPersistentSet  clojure.core/disj (core.clj:1476)
ti.repl-init=> (clojure.set/intersection [0] [:foo])
[0]

See comments on CLJ-1953

If you want an off-the-shelf compatible replacement for clojure.set functions that are identical in behavior, except they perform run-time type checks of the arguments you provide to them, and throw an exception if they have the wrong types (e.g. not sets for union, intersection, difference, subset?, and superset?), consider using the fungible library: https://github.com/jafingerhut/funjible

CLJ-810 was similar, except it was for function clojure.set/difference. That one was declined with the comment "set/difference's behavior is not documented if you don't pass in a set." I do not know what core team will judge ought to be done with this ticket, but wanted to provide some history.

Dynalint [1] and I think perhaps Dire [2] can be used to add dynamic argument checking to core functions.

[1] https://github.com/frenchy64/dynalint
[2] https://github.com/MichaelDrogalis/dire

Now that `set` is faster for sets, I think we could actually add checking for sets in some places where we might not have before. So, it's worth looking at with fresh eyes.

Back in 2009 I submitted a patch to the set functions with explicit `set?` checks and Rich's response was "the fact that these functions happen to work when the second argument is not a set is an implementation artifact and not a promise of the interface, so I'm not in favor of the set? testing or any other accommodation of that." Not sure if that is still accurate though.

If you want an off-the-shelf compatible replacement for clojure.set functions that are identical in behavior, except they perform run-time type checks of the arguments you provide to them, and throw an exception if they have the wrong types (e.g. not sets for union, intersection, difference, subset?, and superset?), consider using the fungible library: https://github.com/jafingerhut/funjible

Not Relevant

This has been raised a number of times. See CLJ-1682, CLJ-810.

I do not see set/union being covered in the tickets you mentioned.

Furthermore, this issue differs from the intersection bugs in a few ways important ways:

1. It silently returns data that is the wrong type, and which contains the wrong values.
2. It never raises an exception.

But it does share the following bugs with the intersection problem:

1. This behavior is not only type dependent, but data dependent. It will happen to work depending on the lengths of the given sets.
2. It isn't even documented that this function expects sets.
3. It runs directly contrary to the definition of the mathematical function it purports to represent.

I only caught this bug in my own code because I hand inspected the result. I had just assumed that set/union would do the right thing, and was deeply surprised when against both definition and documentation it did not.

I am sympathetic to your desires, Ashton, but have no new arguments that might convince those who decide what changes are made to Clojure that it would be a good enough idea to do so.

I would point out an answer to one of your comments: "It isn't even documented that this function expects sets." It seems to me from past comments that the point of view of the Clojure core team is that this is documented, e.g. "Return a set that is the union of the input sets" tells you what clojure.set/union does when you give it sets as arguments. It specifies nothing about what it does when you give it non-set arguments, so it is free to do anything at all in those cases, including what it currently does.

If you want an off-the-shelf compatible replacement for clojure.set functions that are identical in behavior, except they perform run-time type checks of the arguments you provide to them, and throw an exception if they have the wrong types (e.g. not sets for union, intersection, difference, subset?, and superset?), consider using the fungible library: https://github.com/jafingerhut/funjible

clojure 1.9, mac osx, java 1.8

This gist shows the above code with better formatting https://gist.github.com/jeroenvandijk/28c6cdd867dbc9889565dca92673a531

This can quickly be traced down to :clojure.core.specs/arg-list which is speced as a (s/and <regex> vector?). When unforming, it doesn't create a vector.

Thinking about it, a vcat would be nice for this and similar cases.

Hello, any news ?

Nope.

Dupe of CLJ-1430

Dupe as noted in the comments

tested on CentOS 6

The Scala community found this issue a while ago, and now the compiler has a max-classfile-name parameter (defaulting to 255). Hashing is used when the limit is exceeded. Maybe we should consider something similar?

I tried clojure.core.match with 13 patterns and the compiliation failed under Windows. I assume this problem is the root cause of it.

Some more info on that:

A colleague of mine just ran into that problem because he's using Linux / eCryptfs (where the limit of 143 is rather small, compared to our FileVault encrypted macOS used otherwise): see https://bugs.launchpad.net/ecryptfs/+bug/344878.

However, Clojure's not alone with that problem, Scala is also hit hard: https://issues.scala-lang.org/browse/SI-3623

There's no "easy" solution to this, and truncating the filename (as Scala does) bears a lot of other problems, obviously.

For all of you bitten by this problem, one possible workaround might be the one proposed by Mario Pastorelli (https://issues.scala-lang.org/secure/ViewProfile.jspa?name=melrief) in comment https://issues.scala-lang.org/browse/SI-3623?focusedCommentId=76104&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-76104:

Use an unencrypted filesystem to temporarily store classfiles, maybe by having a tempFS to keep stuff in memory. Not the best way, because we do not like leaving important stuff unencrypted, ...

Hello,

I run into the same problem while using clojure.core.match. I macroexpand the function definition and observe that the macro-expanded definition is deeply nested.

I think one way to solve it is to provide an option to wrap the class file as a jar with some shorter file name so that the the class name can remain the same (zipped file name can be as long as you like, right?). WDYT?

Btw, I am using clojure 1.9.0 so I think we should say that this bug affects 1.9 as well.

I come up with a proof-of-concept patch. The compiler now outputs jar instead of class if the class name is longer than 255. The jar name is simply a (left) truncation of the class name.

Suppose *compile-path* is set to "build", then you need to add build/* to your class path, so that the jars can be found.

This patch is only a proof of concept, ideally all classes with long name should be put into one big jar to avoid having to decompress many files. Also, the user should be able to specify *compile-name-max* and *compile-jar-name*. Finally, the code is quite ugly, I should have spitted things into several functions.

Alex - we're not going to output jars. This is at odds with many facets of the Clojure runtime.

Sorry if this comment is off topic, but the original example is a bit confusing to me. Maybe it should be:

(defmacro nestfn [n & body]
  (if (> n 0)
    `(fn [] (nestfn ~(dec n) ~@body))
    `(do ~@body)))

That way (trampoline (nestfn 10 "foo")) would return "foo". However, I do get a CompilerException java.lang.StackOverflowError for n=1000 on macOS.

@Alex Miller Hello! Would a patch with Scala-like approach (hashing of name in case it's too long) be considered? As it breaks binary compatibility this workaround would definitely be disabled by default, but it would be possible to enable name hashing by using new compiler option. This approach would definitely help in our particular case, but I'm not sure if it's useful/generic enough to be included in compiler, so I haven't started working on patch yet.

Not going to make any breaking changes. Plan should be to switch second strategy when current strategy doesn’t work.

You can use s/describe or s/form to grab the source of a spec now, btw.

The following works in my tests. (For testing I used in-ns, @#'registry-ref, #'ns-qualify)).

The approach is to set the registry item metadata after a def. It is not enough to set metadata on the def'd value because it is subsequently altered inside def.

(ns clojure.spec)
(alias 'c 'clojure.core)

(defmacro def
  [k spec-form]
  (let [k (if (symbol? k) (ns-qualify k) k)
        m (assoc (meta &form) :file *file*)]
    `(do
       (def-impl '~k '~(res spec-form) ~spec-form)
       (swap! registry-ref update '~k vary-meta c/merge ~m)
       '~k)))

(defmacro fdef
  [fn-sym & specs]
  (let [k (ns-qualify fn-sym)
        m (assoc (meta &form) :file *file*)]
    `(do
       (clojure.spec/def ~fn-sym (clojure.spec/fspec ~@specs))
       (swap! registry-ref update '~k vary-meta c/merge ~m)
       '~k)))

You can use s/describe or s/form to grab the source of a spec now, btw.

Yes, that's nice except for longer specs when line wrapping and indentation would help.

Note that current :line and :column meta are not pointing to the place where the spec was defined but to the clojure/spec.clj file, e.g. second example (c.c/let) points to fspec-impl

I've been looking into fixing this and for specs that aren't ident? we can use metadata or the map describing regex? specs.

For ident? specs I'm not sure how to handle the additional data — it could be stored in some other location of the registry which might be a viable path.

With any of these approaches it's unclear to me how to integrate this information into the return value of get-spec in a consistent way since it's return value may not be maps (therefore not extensible.)

Storing them in a different location in the registry would work as mentioned before but then we would need another function or protocol method to get information about source file/line/column.

Fix typo: later => latter.

+100 for improving error messages.

Albeit unlikely, since `throwArity()` is a public method, removing it risks breaking external code.

Less importantly, the resulting sentence looks a bit awkward with the three colons:
"Wrong number of args (3) passed to: keyword: :my.ns/foo"

Alternatively:
"Wrong number of args (3) passed to: keyword :my.ns/foo"
"Wrong number of args (3) passed to: :my.ns/foo"

The latter parallels the result when calling a non-keyword function:
"Wrong number of args (3) passed to: my.ns/bar"

Also, actually there is a typo artity-exceptions in the test.

And I would agree with Alexander's comment, should leave existing throwArity().

Attach updated patch.

I've attached an updated patch that fixes the type in the deftest declaration and formats the exception message in the way Alexander suggested.

Alex - should I add a Java comment to explain why the 0-arity version of Keyword.throwArtity() exists? Something like:

/**
 * @deprecated CLJ-2350 This function is no longer called, but has not been removed to maintain the public interface.
 */

Reflecting on the public interface, perhaps we could make the new function, throwArity(int n), package-private. This would avoid it becoming part of the public API of clojure.lang.

yes and yes

Added patch making throwArity(int) package private, and adding deprecation docstring.

The final invoke takes a variadic as the final arg so it's actually 21+, not 21. Could count the args array. The patch has also drifted from master and needs to be rebased.

Added patch that improves the message for the 21+ arg case.

There are two more places in the code that have the same bug in the 21+ arg case:

https://github.com/clojure/clojure/blob/71511b7800e18c83377a322f43585a853b303698/src/jvm/clojure/lang/RestFn.java#L4078
https://github.com/clojure/clojure/blob/71511b7800e18c83377a322f43585a853b303698/src/jvm/clojure/lang/AFn.java#L140

Would you like an additional patch to cover those cases?

Sure, separate ticket though.

Hi Alex, any update on this ticket? I've updated the patch to the latest master as requested, so it should be good to merge now.

I’ll get to it next time I cycle through.

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?

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.

Attaching a patch that makes realized? return true for non-pending xs.

Scratch that - patch is no good.

Attaching a patch that makes realized? return true for non-pending xs. Add tests for the IPending types.

The basic idea here is that instrument is designed to verify that a function is invoked correctly. For determining whether the internals of a function are correct, you can use `check`.

This has been discussed at great length, both on the core team and in the broader community and we’re familiar with all the arguments. While I’m not ruling out the possibility that we will do something in this area in the future, we are not currently planning on changing this, so I’m going to decline this for now. I’ll repeat, saying no to this now does not prevent us from saying yes later.

https://dev.clojure.org/display/community/Creating+Tickets
https://groups.google.com/forum/#!topic/clojure/RLQBFJ0vGG4/discussion
There's also some commentary from Rich in Slack, but I don't have a link handy.
("we should do X" tickets are almost universally declined.)
dup of https://dev.clojure.org/jira/browse/CLJ-1961

Sorry, I've only searched through open issues before creating this ticket.

Is it at all possible to keep this issue open so people could vote for it and subscribe to it, since (as Alex stated) it's not set in stone yet?

(The thing is, I was confused by this behavior and asked on irc and elsewhere, and was told by several people that it's weird and looks like a bug. I've filed it as a "feature" since the doc clearly states that's the intended behavior. Also, it's not quite clear how to use check to verify correctness of a function for a specific subset of possible inputs (think checking the fn in repl as you develop it) - maybe the spec guide could elaborate a bit more on what the supposed workflow should look like? Maybe that'd clear up some confusion; though, I understand that's out of the scope.)

As I said, we're aware of the request and why people want it. From a core team perspective, we don't need an issue to track it.

Use `check` if you want to use property based testing to verify the correctness of a function with respect to its spec. Generally if you want to test specific possible inputs, most people use example-based tests with clojure.test instead - that's not in the guide as it doesn't really have anything to do with spec. It is possible to override the args generator with a set spec of explicit inputs, but I'm not sure if the effort of that is worth the work most of the time.

Well, fair enough!

Re example-based tests: what I had in mind is 1) calling a function (presumably in repl) with a specific input to see if the output still conforms to :ret and :fn specs; 2) testing a function A which calls spec'ed functions B1..Bn to see which of those fail their :ret/:fn specs (essentially it gives you free unit tests for each integration test). I agree that it's theoretically possible to do that in vanilla clojure.spec with a custom generator, but that certainly doesn't worth the effort.

Anyway, I guess you've already heard this argument, so I'll stop there.

Clojure 1.10.0-alpha6

Hello, please note that the Cognitect blog article http://blog.cognitect.com/blog/2016/8/9/focus-on-spec-predicates still mentions "bigdec?", which is confusing for people googling for the correct predicate.

I removed it from the blog post.

compile-if is gone, this issue is now obsolete.

[org.clojure/clojure "1.9.0-alpha14"]

I don't think we are interested in turning spec into a transformation engine via conformers, so I suspect we are probably not interested. However, I'll leave it for Rich to assess.

Currently, Plumatic Schema is the tool used at the borders. Now, people are starting to move to Spec and it would really bad for the Clojure Web Developement Story if one had to use two different modelling libraries for their apps. If Spec doesn't want to be a tranformation engine via conformers, I hope for the Alternative suggestion to allow 3rd parties to write this kind of extensions: exposing Specs as Records/Types instead of reified protocols would do the job?

I could see why the Clojure core developers might not want Spec to support this kind of coercion, but the practical reality is that someone will have to. If it isn't in Spec itself, it'll have to be done libraries built upon it like Tommi's.

The use case here is: I have a conf file that is YAML. I'm parsing the YAML using a Clojure library, turning it into a map. Now I have to validate the map, but YAML doesn't support keywords, for example, and the settings structure goes directly into Component/Mount/etc as part of the app state, so it makes sense to run s/conform on it as the first step in app startup after reading configuration. Add to this the possibility of other methods of merging in configuration (env vars, .properties files, etc) and this coercion will be necessary somewhere.

Any news on assessing this? I would be happy to provide a patch or a link to a modified clojure.spec with samples on usage with the 3-arity conform in it. Some thinking aloud: http://www.metosin.fi/blog/clojure-spec-as-a-runtime-transformation-engine/

Rich hasn't looked at it yet. My guess is still that we're not interested in this change. While I think some interesting problems are described in the post, I don't agree with most of the approaches being taken there.

Why not just use s/or (or s/alt) and then dispatch on the tag. Something like:

(s/def ::id (s/and (s/or :int integer?
                         :str string?)
                   (s/conformer (fn [[tag x]]
                                  (case tag
                                    :int x
                                    :str (Integer/parseInt x))))))

I use that pattern quite a bit in https://github.com/sbelak/huri and with a bit of syntactic sugar it works quite well.

Simon that will not work if you are trying to conform to specs from third parties though. One of the points of this suggestion is that third parties would be able to write their own conformers to existing specs without redefining those specs.

Thanks for the comments. I would be happy to provide a patch / sample repo with the changed needed for this, in hope that it would help to decide if this could end up in the spec or not. What do you think?

Below is a sample of initial spec-integration into ring/http libs, using spec-tools. For now, one needs to wrap specs into spec records to enable the 3-arity conforming. This is boilerplate I would like to see removed. With this change, it should work out-of-box for all (3rd party) specs.

(require '[compojure.api.sweet :refer :all])
(require '[clojure.spec.alpha :as s])
(require '[spec-tools.core :as st])

;; to enable 3-arity conforming
(defn enum [values]
  (st/spec (s/and (st/spec keyword?) values)))

(s/def ::id int?)
(s/def ::name string?)
(s/def ::description string?)
(s/def ::size (enum #{:L :M :S}))
(s/def ::country (st/spec keyword?) ;; to enable 3-arity conforming
(s/def ::city string?)
(s/def ::origin (s/keys :req-un [::country ::city]))
(s/def ::new-pizza (st/spec (s/keys :req-un [::name ::size ::origin] :opt-un [::description])))
(s/def ::pizza (st/spec (s/keys :req [::id] :req-un [::name ::size ::origin] :opt-un [::description])))

;; emits a ring-handler with input & output validation (& swagger-docs)
;; select conforming based on request content-type (e.g. json/edn) + strip-extra keys from maps
(context "/spec" []
  (resource
    {:coercion :spec
     :parameters {:body-params ::new-pizza}
     :responses {200 {:schema ::pizza}}
     :post {:handler (fn [{new-pizza :body-params}]
                       (ok (assoc new-pizza ::id 1))}}))

Intended to create internal PR in my fork of clojure.spec, but ended up doing a real DUMMY PR for the actual repo. Well, here it is anyway:

https://github.com/clojure/spec.alpha/pull/1

Happy to finalize & create a patch into Jira if this goes any further.

comments welcome. here's a sample test for it:

(deftest conforming-callback-test
  (let [string->int-conforming
        (fn [spec]
          (condp = spec
            int? (fn [_ x _]
                   (cond
                     (int? x) x
                     (string? x) (try
                                   (Long/parseLong x)
                                   (catch Exception _
                                     ::s/invalid))
                     :else ::s/invalid))
            :else nil))]

    (testing "no conforming callback"
      (is (= 1 (s/conform int? 1)))
      (is (= ::s/invalid (s/conform int? "1"))))

    (testing "with conforming callback"
      (is (= 1 (s/conform int? 1 string->int-conforming)))
      (is (= 1 (s/conform int? "1" string->int-conforming))))))

initial work as patch.

Any news on this?

Related https://dev.clojure.org/jira/browse/CLJ-2251

Hello, any news ?

We won’t look at this at least until we do the next batch of implementation changes. I continue to think we will most likely decline this.

[org.clojure/spec.alpha "0.1.134"]

Renamed the issue. Instead of Keyword argument, it should take a function to walk the spec to support arbitrary walking applications.

hello, any news ?

No plans to look at this before the next batch of implementation changes, so it will be a while.

java version "11-ea" 2018-09-25
Java(TM) SE Runtime Environment 18.9 (build 11-ea+21)
Java HotSpot(TM) 64-Bit Server VM 18.9 (build 11-ea+21, mixed mode)
OS : Ubuntu

I applied the patch by Will Cohen for core.rrb-vector and Travis builds are green : https://travis-ci.org/tirkarthi/clojure/builds/402141339

Relevant commit : https://github.com/tirkarthi/clojure/commit/63dab8e6cb702a6b0c5b279721bee7eff0aba44f
Commit as patch : https://github.com/tirkarthi/clojure/commit/63dab8e6cb702a6b0c5b279721bee7eff0aba44f.patch

Credits to Will Cohen

Edit : The change seems to introduce source incompatibility

Ref : https://bugs.openjdk.java.net/browse/JDK-8060192?focusedCommentId=14194092&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14194092
Relevant commit : http://hg.openjdk.java.net/jdk/jdk11/rev/3ef0862bbb3d

Thanks

We added JDK 11 EA to our test build matrix yesterday but I had not had the chance to file this yet (https://build.clojure.org/job/clojure-test-matrix/), so thanks! I think it's worth looking into Compiler/Reflector changes to see if this is fixable at a different level.

I don't see that this means that this or other versions of Clojure would not work on Java 11. The bytecode produced is still compatible; this is an issue caused by a JDK lib change.

That's right, AOT'ed bytecode will still work on JDK11, but you can't build gvec on JDK11 because there is a new overload to toArray that requires a type hint. We could add the disambiguation hint now in anticipation of the change.

I don't think it's a new overload - the arity existed, but now has a default implementation, right?

That's right, it's a new method with the same name and arity, but different arg types. Users can override default interface methods, so from our perspective we need to have a type hint to disambiguate the new method when reifying:

toArray(T[] a)
toArray(IntFunction<T[]> generator) # added

(This method could have been added with a new name!)

It also always fails when referring to a predicate function, so the only way to get it to work is to define another spec and refer to that.

user=> (defn a<b [a b] (< a b))
#'user/a<b
user=> (s/def ::e (s/merge (s/keys :req [::a ::b]) a<b))
:user/e
user=> (s/explain-data ::e {::a 1 ::b 2})
#:clojure.spec.alpha{:problems ({:path [], :pred user/a<b, :val #:user{:a 1, :b 2}, :via [:user/e], :in []}), :spec :user/e, :value #:user{:a 1, :b 2}}
user=> (s/explain-data ::e {::a 1 ::b 0})
#:clojure.spec.alpha{:problems ({:path [], :pred user/a<b, :val #:user{:a 1, :b 0}, :via [:user/e], :in []}), :spec :user/e, :value #:user{:a 1, :b 0}}

#(< (::a % ::b %)) doesn't seem right? Does #(< (::a %) (::b %)) work?

Good catch, however, no #(< (::a %) (::b %)) does not work inline. I updated the example with to reflect that.

Also, FWIW, I'd narrowed down the problem elsewhere, before my typo Just so happened that I got the same answer with and without the typo.

It does work if you wrap the predicate in {s/spec}:

user=> (s/def ::f (s/merge (s/keys :req [::a ::b]) (s/spec #(< (::a %) (::b %)))))
:user/f
user=> (s/explain-data ::f {::a 1 ::b 2})
nil
user=> (s/explain-data ::f {::a 1 ::b 0})
#:clojure.spec.alpha{:problems ({:path [], :pred (clojure.core/fn [%] (clojure.core/< (:user/a %) (:user/b %))), :val #:user{:a 1, :b 0}, :via [:user/f], :in []}), :spec :user/f, :value #:user{:a 1, :b 0}}

I'm unable to reproduce any of this.

In the issue description, the predicate #(< (::a %) (::b %)) require a to be less than b, so it is expected that {::a 1 ::b 0} doesn't match the spec.

In the first comment, the predicate a<b is invalid, since it accepts two numbers instead of a single map.

The following snippet works as expected too (there's no difference between anonymous pred, named pred and a pred wrapped in s/spec):

(s/def ::a number?)
(defn good? [m] (contains? m :a))

(def s1 (s/merge (s/keys :req-un [::a]) good?))
(def s2 (s/merge (s/keys :req-un [::a]) #(contains? % :a)))
(def s3 (s/merge (s/keys :req-un [::a]) (s/spec #(contains? % :a))))

(s/valid? s1 {:a 42}) ;=> true
(s/valid? s2 {:a 42}) ;=> true 
(s/valid? s3 {:a 42}) ;=> true

I believe the issue can be closed.

When I updated the example after Alex's comment, I put the message in the wrong place. I just updated it again with the messages correctly aligning w/ the output. jcr, I do see that your examples work, but mine still do not. So this is not quite as general as all anonymous predicates, but there is still surprising behavior.

That said, s/merge docs say it supports keys specs, and does not claim to support any other type of predicate, so if you want to close it on those grounds, have at it.

Ah, now I see what you mean. Sorry for jumping to conclusions.

So the actual problem here is that s/explain inconsistently reports inputs that are valid according to s/conform when s/merge is used with a predicate not wrapped in a s/spec call. Here's the minimal example I've managed to come up with:

(ns test
  (:require [clojure.spec.alpha :as s]))

(defn pred [m] (contains? m :a)) 
(s/def ::s (s/merge pred))

(def good {:a 42})
(def bad  {})

;; as expected
(s/conform ::s good) ;=> {:a 42}
(s/conform ::s bad)  ;=> :clojure.spec.alpha/invalid

;; !!!: first one should NOT fail
(s/explain-str ::s good) ;=> "val: {:a 42} fails spec: :test/s predicate: pred\n" 
(s/explain-str ::s bad)  ;=> "val: {} fails spec: :test/s predicate: pred\n"

;; explicitly wrap pred in spec
(s/def ::s* (s/merge (s/spec pred)))

;; now, works as expected
(s/explain-str ::s* good) ;=> "Success!\n"
(s/explain-str ::s* bad)  ;=> "val: {} fails spec: :test/s* predicate: pred\n"

I would suggest updating the title and the description accordingly, if you don't mind?

As far as I can tell, the problem is that merge-spec-impl simply calls explain-1 with the given preds, and explain-1 always returns problems if (spec? pred) is false; merge-spec-impl should probably call specize on its preds, similar to how it's done in the tuple-impl:

(let [specs (delay (mapv specize preds forms)) ...] ...)

Java 8