<< Back to previous view

[CLJ-1081] REPL binding not working that works with with-bindings Created: 30/Sep/12  Updated: 23/Feb/17  Resolved: 23/Feb/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Steven Devijver Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: repl


 Description   

This works as expected:

java -jar clojure-1.4.0.jar -e "(do (require 'clojure.repl) (.setDynamic #'clojure.repl/print-doc) (with-bindings {#'clojure.repl/print-doc str} (eval '(clojure.repl/doc println))))"

Output:

"{:ns #<Namespace clojure.core>, :name println, :arglists ([& more]), :added \"1.0\", :static true, :doc \"Same as print followed by (newline)\", :line 3325, :file \"clojure/core.clj\"}"

But the same thing does not work in the REPL:

java -jar clojure-1.4.0.jar -e "(do (require 'clojure.repl) (.setDynamic #'clojure.repl/print-doc) (clojure.main/repl :init (fn [] {#'clojure.repl/print-doc str}))))"

Output for Output of {{(doc println)}}:

user=> (doc println)
-------------------------
clojure.core/println
([& more])
Same as print followed by (newline)
nil
user=>




 Comments   
Comment by Steven Devijver [ 01/Oct/12 5:51 AM ]

Found a work-around:

java -jar clojure-1.4.0.jar -e "(do (require 'clojure.repl) (.setDynamic #'clojure.repl/print-doc) (with-bindings {#'clojure.repl/print-doc str} (clojure.main/repl)))))"

I'm still not sure whether the method above using :init should or should not work.

Comment by Alex Miller [ 23/Feb/17 3:40 PM ]

You shouldn't expect to be able to reconfigure the repl from within itself like this. I think the workaround of creating a new differently configured repl is correct.





[CLJ-929] Accessing Java property starting with _ has issues in 1.4 Created: 07/Feb/12  Updated: 23/Feb/17  Resolved: 23/Feb/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: defrecord

Approval: Triaged

 Description   

When attempting to use interop syntax with symbols which aren't legal Java names (such as deleted?), the names are mangled a bit. That's necessary, of course, and the method of munging can be internal to the compiler. However, the behavior when munging changed a little between 1.3 and 1.4 beta1. Obviously the specifics of munging are something I should avoid relying on, but the way it changed looks like an accident or a bug even so.

The use-case I ran into is that defrecords contain a field named __meta for tracking their metadata. In both 1.3 and 1.4 you can get at that field with (. record __meta), which avoids munging. But on 1.3 (. record --meta) also accesses it (translating each - to a _), while on 1.4 (. record -meta) works and (. record --meta) doesn't.

Actually, looking at line 883 of Compiler.java, it looks like this may be related to the (. foo -property) syntax ported from CLJS, and indeed (. record ---meta) works, I guess by reducing to an "old style" (. record --meta). So that clears up why --meta fails: it's looking for __meta. I'm still not clear on why (. record -meta) works, though.

So it looks like the - prefix for properties is not 100% backwards-compatible like it seemed to be. Is this an issue we need to fix, or is the recommendation simply to never have fields that start with - or _?



 Comments   
Comment by Fogus [ 09/Feb/12 2:33 PM ]

Is this a general problem with fields starting with _ or just fields named __meta as in (defrecord [__meta] ...)

Comment by Alan Malloy [ 09/Feb/12 3:01 PM ]

It's a general issue. (defrecord [__meta]) actually breaks immediately, because the record mechanism itself generates a field named __meta, but any field named with a - or _ prefix has this issue.

user=> (defrecord Foo [-blah])
user.Foo
user=> (.-blah (Foo. 1))
IllegalArgumentException No matching field found: blah for class user.Foo clojure.lang.Reflector.getInstanceField (Reflector.java:289)

Comment by Alex Miller [ 23/Feb/17 3:32 PM ]

The . syntax is interop and should always be used with Java-style field names. If other things work, that's undocumented and subject to change. You can access fields by using either . (which can also refer to methods) or .- (for field access only if there is ambiguity) - also see https://clojure.org/reference/java_interop.

The munging style for internal record field names should be considered an implementation detail, although maybe one it's useful to know about.

For the internal _meta example, which will become a Java field named "_meta":

;; Things that should (and do) work for interop:
(.__meta foo)
(. foo __meta)
(.-__meta foo)
(. foo -__meta)

Anything with --meta is undefined and unsupported.

For a field that starts with -, the following should be expected to work for the munged name ("_blah"):

(defrecord R [-blah])
(def r (->R 10))

(._blah r)
(. r _blah)
(.-_blah r)
(. r -_blah)

Anything Java interop stuff with -blah is undefined and unsupported.





[CLJ-190] enhance with-open to be extensible with a new close multimethod Created: 13/Sep/09  Updated: 23/Feb/17  Resolved: 23/Feb/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: io


 Description   

Discussion: http://groups.google.com/group/clojure/browse_thread/thread/8e4e56f6fc65cc8e/618a893a5b2a5410

Currently, with-open calls .close when it's finished. I'd like it to have a (defmulti close type) so it's behavior is extensible. A standard method could be defined for java.io.Closeable and a :default method with no type hint. I've come across a few cases where some external library defines what is essentially a close method but names it shutdown or disable, etc., and adding my own "defmethod close" would be much easier than rewriting with-open. This would also allow people to eliminate reflection for classes like sql Connection that were created before Closeable.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 4:30 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/190
Attachments:
clojure-190-with-open.patch - https://www.assembla.com/spaces/clojure/documents/ca27R6Ojur3PQ0eJe5afGb/download/ca27R6Ojur3PQ0eJe5afGb

Comment by Assembla Importer [ 24/Aug/10 4:30 AM ]

mikehinchey said: [file:ca27R6Ojur3PQ0eJe5afGb]: fix adds close method and tests

Comment by Assembla Importer [ 24/Aug/10 4:30 AM ]

mikehinchey said: Note, I only defined methods for :default (reflection of .close) and Closeable, not sql or the numerous other classes in java that should be Closeable but are not. Maybe clojure.contrib.sql and other such libraries should define related close methods.

Comment by Assembla Importer [ 24/Aug/10 4:30 AM ]

richhickey said: I want to hold off on this until scopes are in

Comment by Tassilo Horn [ 23/Dec/11 6:50 AM ]

Probably better implemented using a protocol. See http://dev.clojure.org/jira/browse/CLJ-308

Comment by Alex Miller [ 23/Feb/17 3:13 PM ]

Superseded by CLJ-308





[CLJ-2117] Support typical polymorphic maps in spec more directly Created: 23/Feb/17  Updated: 23/Feb/17  Resolved: 23/Feb/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: spec


 Description   

Problem: A common scenario is that one wants to spec this:

(def a-poly-map
  {::type :t1
   <kvs required/optional in t1>
   <kvs required/optional in all>
   })

To do this, you usually write lots of boiler plate code like this:

(s/def ::type #{:t1 <more-types>})

(defmulti type-dispatch ::type)
(defmethod type-dispatch :t1 [_] (s/keys :req [<t1-keys>]))
<more defmethods for more-types>

(s/def ::poly-map (s/merge (s/keys :req [::type <poly-map-keys-in-all>])
                           (s/multi-spec type-dispatch ::type))

As a workaround one can (and we did) of course write a macro to write all that code.

From my experience this usecase is the 90% multi-spec usecase and thus I'd prefer if a more convenient to use spec existed.

Proposed solution: Implement this spec:

(s/def ::poly-map (s/merge (s/keys :req [<poly-map-keys-in-all>]
                           (s/poly-map ::type
                             :t1 {:req [<t1-keys>]} ; :req, :opt etc. as in s/keys
                             <more requirements for more types>)))

Limitations: This syntax does not support to reuse existing specs in for dispatch values (:t1, :t2, ...) intentionally, because it seems an non-existing usecase to spec incomplete s/keys for such maps separately anyway.

Further enhancement: While the <poly-map-keys-in-all> usecase is rare, this syntax could also directly support it:

(s/def ::poly-map (s/poly-map ::type
                    {:req [<poly-map-keys-in-all>]} ; optional
                    :t1 {:req [<t1-keys>]}
                    <more requirements for more types>))

This new spec would automatically check for the presence of ::type and conform/explain like a regular s/keys.



 Comments   
Comment by Alex Miller [ 23/Feb/17 11:50 AM ]

Thanks, but I don't think we expect to do anything like this.

Comment by Leon Grapenthin [ 23/Feb/17 12:03 PM ]

Why?

Comment by Alex Miller [ 23/Feb/17 12:51 PM ]

Because spec provides the tools to do it already.

Comment by Leon Grapenthin [ 23/Feb/17 1:55 PM ]

This ticket is asking for a syntactic simplification of a typical use case. I understand that there already is s/multi-spec. s/multi-spec however is a generic and in comparison quite advanced tool that is not tied to maps. Thus it requires a substantial amount of boilerplate to "do it".

After much practical experience with spec in my observation - and I do assume others would agree - this is the most common application for it.

There are many things in clojure.core which aren't the only "tools to do it" for the exact same reason, so please reconsider not closing this right away.

Comment by Alex Miller [ 23/Feb/17 2:32 PM ]

My job is to weigh the balance of several factors and decide if an enhancement request seems reasonably likely to remain as an open ticket and at some point be further evaluated.

Some factors I try to think about:

  • Is this a good problem? Does it describe a pain point that many people are likely to encounter? You claim it is "most common" use but seems way too early to judge this (from what I've seen, I wouldn't agree right now).
  • Is the pain significant enough that it prevents you from doing your work? In this case, clearly not - you've already built it and it would be easy to release it independently from core.
  • Is this a fundamental (simple) feature that belongs in core? Clojure has a small library and wishes to keep it that way. There are ample examples where this constraint was not considered enough in Clojure. From what I can tell, this is appears to be a composite of existing things, not a fundamental part.
  • If we were to solve this problem in core, is this the proposed solution a likely way we would do it? Based on things we've talked about during dev, my suspicion is, probably not.

On balance, it does not seem to be something worth leaving open in the tracker right now. But as the saying goes, "no is temporary, yes is forever". If a while down the road it's clear that this is a prominent problem that needs attention, there is nothing preventing us from considering it again (presumably with more data and experience at that time).

Comment by Leon Grapenthin [ 23/Feb/17 3:11 PM ]

Thanks for explaining the decision. I understand that you have priorities and probably a better plan to address this problem in the future.





[CLJ-200] Extend cond to support inline let, much like for Created: 18/Oct/09  Updated: 23/Feb/17  Resolved: 01/Jul/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Mark Engelberg
Resolution: Declined Votes: 11
Labels: None

Attachments: File clj-200-cond-let-clauses-fixed-test-v2.diff     Text File clj-200-cond-let-clauses-fixed-test-v2-patch.txt    
Patch: Code and Test

 Description   

I find it occasionally very useful to do a few tests in a cond, then introduce some new symbols (for both clarity and efficiency) that can be referenced in later tests (or matching expressions). This parallels similar functionality inside the for macro, where the :let keyword is matched against a vector of symbol bindings and forms an implicit let around the remainder of the comprehension.

I'll be adding a patch for this shortly.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 1:51 PM ]

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

Comment by Assembla Importer [ 24/Aug/10 1:51 PM ]

hlship said: Trickier than I thought because cond is really wired into other fundamentals, like let.

Comment by Assembla Importer [ 24/Aug/10 1:51 PM ]

cgrand said: Howard, what do you think of http://gist.github.com/432712 ?

Comment by Mark Engelberg [ 23/Nov/12 2:33 AM ]

Patch cond-let-clauses.diff on 23/Nov/12 adds inline :let clauses to cond, implementing CLJ-200. The code is based off of code by cgrand, with some tweaks so the implementation only relies on constructs defined earlier in core.clj, since when cond is defined, things aren't yet fully bootstrapped. Also added a test to control.clj.

Comment by Christophe Grand [ 23/Nov/12 3:06 AM ]

Some comments: the docstring is missing, I believe you don't have to modify the original cond (except the docstring maybe), just redefine it later on once most of the language is defined – a bit like what is done for let for example.

There is still the unlikely eventuality that some code uses :let as :else. What about shipping a cond which complains on keywords (in test position) other than :else?

Comment by Mark Engelberg [ 23/Nov/12 3:47 AM ]

cond-let-clauses-with-docstring.diff contains the same patches as cond-let-clauses, but includes the original docstring for cond along with an additional sentence about the :let bindings.

Comment by Mark Engelberg [ 23/Nov/12 3:54 AM ]

Cgrand, I did see your example of redefining cond after most of the language is defined, but since I was able to figure out how to do it in the proper place, that makes the :let bindings available for users of cond downstream and avoids any unforeseen complications that might come from rebinding.

As for your other point, I think it is highly improbable that someone would have used :let in the :else position. However I can imagine someone intentionally using something like :true or :default. I think the idea of warning for other keywords is actually more likely to cause complications than the unlikely problem it is meant to solve.

I did resubmit the patch with the docstring restored. Thanks for pointing out that problem. I'm excited about this patch – I use :let bindings within the cond in my own code all the time. Thanks again for the blog post that started me on that path.

Comment by Christophe Grand [ 23/Nov/12 4:13 AM ]

True, it's :unlikely for :let to happen.
However once :let is officially blessed, it may be better to provision for future other "special" keywords and thus to warn on "unsupported" keywords. Plus it will help out-of-order typists (like myself) to catch earlier a :elt instead of a :let
This is only my point of view. Thanks for trying to get :let in cond supported.

Comment by Andy Fingerhut [ 29/Nov/12 8:46 PM ]

Mark, could you remove the obsolete earlier patch now that you have added the one with the doc string? Instructions for removing patches are under the heading "Removing Patches" on this page: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Mark Engelberg [ 29/Nov/12 10:50 PM ]

Done.

Comment by Andy Fingerhut [ 30/Nov/12 1:24 AM ]

I haven't figured out what is going wrong yet. I can apply the patch cond-let-clauses-with-docstring.diff to the latest Clojure master just fine. I can do "ant jar" and it will build a jar. When I do "ant", it fails with the new test for cond with :let, throwing a StackOverflowException. I can enter that same form into the REPL and it evaluates just as the test says it should. I can comment out that new test and all of the rest pass. But the new test doesn't pass when inside of the control.clj file. Anyone know why?

Comment by Christophe Grand [ 30/Nov/12 4:54 AM ]

It's because of the brutal replacement performed by test/are: the placeholders for this are form are x and y but in Mark's test there are used as local names and are tries to substitute them recursively...
If one changes the local names to a and b for example it works.

Comment by Mark Engelberg [ 02/Dec/12 8:20 AM ]

cond-let-clauses-fixed-test.diff on 02/Dec/12 contains the same patch, but with the x,y locals in the test case changed to a,b so that it works properly in the are clause which uses x and y.

Comment by Mark Engelberg [ 02/Dec/12 8:27 AM ]

On Windows, I can't get Clojure's test suite task to work, either via ant or maven, which has made it difficult for me to verify the part of the patch that applies to the test suite works as expected; I had tested it as best I could in the REPL, using a version of Clojure built with the patch applied, but using this process, I missed the subtle interaction between are and the locals in the test case. Sorry about that. If someone can double-check that the test suite task now works with the newest patch, that would be great, and then I'll go ahead and remove the obsoleted patch. Thanks.

Comment by Andy Fingerhut [ 02/Dec/12 6:29 PM ]

clj-200-cond-let-clauses-fixed-test-v2-patch.txt dated Dec 2 2012 is identical to Mark Engelberg's cond-let-clauses-fixed-test.diff of the same date, except it applies cleanly to the latest Clojure master.

I've verified that it compiles and passes all tests with latest Clojure master as of this date.

Mark, I've made sure to keep your name in the patch, since you wrote it. You should be able to remove your two attachments now, so the screener won't be confused which patch should be examined.

Comment by Andy Fingerhut [ 02/Dec/12 6:31 PM ]

Mark, besides general issues with Windows not being used much (or maybe not at all?) by Clojure developers, there is the issue right now filed as CLJ-1076 that not all tests pass when run on Windows due to CR-LF line ending differences that cause several Clojure tests to fail, regardless of whether you use ant or maven to run them.

Comment by Andy Fingerhut [ 22/Oct/13 8:01 PM ]

clj-200-cond-let-clauses-fixed-test-v2.diff is identical to earlier patch clj-200-cond-let-clauses-fixed-test-v2-patch.txt, except it removes unnecessarily trailing whitespace that causes warnings when applying the patch, and with .diff suffix for easier diff-style viewing in some editors.

Comment by Mark Engelberg [ 01/Jul/16 7:09 PM ]

It was just brought to my attention that this issue was declined today.

FWIW, earlier today I wrote up a Rationale about this issue as part of my effort to address it temporarily at the library level. I hope you will take a look:

https://github.com/Engelberg/better-cond#rationale

Comment by Alex Miller [ 23/Feb/17 3:06 PM ]

This ticket was resolved, but not closed. Just moving this ticket into the right jira state.





[CLJ-2118] extend-type doesn't identify that a protocol is a protocol Created: 23/Feb/17  Updated: 23/Feb/17  Resolved: 23/Feb/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Maurício Eduardo Chicupo Szabo Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: protocols
Environment:

Ubuntu 16.04 x86_64, OpenJDK 8



 Description   

I'm trying to implement this tutorial: http://www.lucagrulla.com/posts/server-sent-events-with-ring-and-compojure/

In one part, I need to extend a core.async type with the following protocol: https://github.com/ring-clojure/ring/blob/master/ring-core/src/ring/core/protocols.clj#L19.

So, I've tried to implement something like this:

src/async_test/protocol_ext.clj
(ns async-test.protocol-ext
  (:require [clojure.core.async :as async]
            [clojure.java.io :as io])
  (:import [ring.core.protocols StreamableResponseBody]
           [clojure.core.async.impl.channels ManyToManyChannel]))

(extend-type ManyToManyChannel
  StreamableResponseBody
  (write-body-to-stream [channel response output-stream]
                        (async/go (with-open [writer (io/writer output-stream)]
                                    (async/loop []
                                                (when-let [msg (async/<! channel)]
                                                  (doto writer (.write msg) (.flush))
                                                  (recur)))))))

But this fails with interface ring.core.protocols.StreamableResponseBody is not a protocol.

Then, I tried to monkey-patch ring: opened a new file with the correct path, and added the following lines on the bottom of the protocol declaration, inside `extend-protocol` call:

src/ring/core/protocols.clj
ManyToManyChannel
  (write-body-to-stream [channel response output-stream]
                        (async/go (with-open [writer (io/writer output-stream)]
                                    (async/loop []
                                                (when-let [msg (async/<! channel)]
                                                  (doto writer (.write msg) (.flush))
                                                  (recur)))))))

Then, it worked. What's happening? Is Clojure ignoring that StreamableResponseBody is a protocol if there's already an `extend-protocol` call?



 Comments   
Comment by Maurício Eduardo Chicupo Szabo [ 23/Feb/17 3:00 PM ]

Okay, my bad. Seems that I need to `:require` the protocol, not `:import` it. Sorry about that...

Comment by Alex Miller [ 23/Feb/17 3:04 PM ]

The issue here is that you are importing StreamableResponseBody as a Java class (really an interface). While the protocol does generate a Java interface, you should use the protocol, not the interface, when you extend. This is confusing because they both have the same name.

So, your ns declaration should instead be:

(ns async-test.protocol-ext
  (:require [clojure.core.async :as async]
            [clojure.java.io :as io]
						[ring.core.protocols :refer [StreamableResponseBody]])
  (:import [clojure.core.async.impl.channels ManyToManyChannel]))

The monkeypatch you tried worked because you were properly referring to the protocol in that case.





[CLJ-2116] Support for selective conforming with clojure.spec Created: 22/Feb/17  Updated: 23/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: spec
Environment:

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



 Description   

Problem

using clojure.spec in runtime border validation supporting multiple exchange formats is hard.

Details

Currently in clojure.spec (alpha-14), conformers are attached to Spec instances at creation time and they are invoked on every conform. This is not very useful in system border validation, where conforming/coercion functions should be selected based on runtime data, e.g. the exchange format.

Examples:

  • a keyword? spec:
    • with EDN, no coercion should be done (it can present Keywords)
    • with JSON, String->Keyword coercion should be applied
    • with String-based formats (CSV, query-params, ...), String->Keyword coercion should be applied
  • a integer? spec:
    • with EDN, no coercion should be done (it can present numbers)
    • with JSON, no coercion should be done (it can present numbers)
    • with String-based formats (CSV, query-params, ...), String->Long coercion should be applied

Here is a more complete example:

(s/def ::id integer?)
(s/def ::name string?)
(s/def ::title keyword?)
(s/def ::person (s/keys :opt [::id], :req-un [::name ::title]))

;; this is how we see the data over different exchange formats
(def edn-person {::id 1, :name "Tiina", :title :boss})
(def json-person {::id 1, :name "Tiina", :title "boss"})
(def string-person {::id "1", :name "Tiina", :title "boss"})

;; here's what we want
(def conformed-person edn-person)

To use this today, one needs to manually create new border specs with different conformers for all different exchange formats. Non-qualified keywords could be mapped in s/keys to work (e.g. ::title => ::title$JSON), but this wont work if fully qualified keys are exposed over the border (like ::id in the example) - one can't register multiple, differently conforming version of the spec with same name.

Suggestion

Support selective conforming in the Spec Protocol with a new 3-arity conform* and clojure.spec/conform, both taking a extra user-provided callback/visitor function. If the callback is provided, it's called from within the Specs conform* with the current spec as argument and it will return either nil or a 2-arity conformer function that should be used for the actual confrom.

Actual conforming-matcher implementations can be maintained in 3rd party libraries, like spec-tools[1].

Using it would look like this:

;; edn
(assert (= conformed-person (s/conform ::person edn-person)))
(assert (= conformed-person (s/conform ::person edn-person nil)))

;; json
(assert (= conformed-person (s/conform ::person json-person json-conforming-matcher)))

;; string
(assert (= conformed-person (s/conform ::person string-person string-conforming-matcher)))

Alternative

Another option to support this would be to allow Specs to be extended with Protocols. 3rd party libs could have a new Conforming protocol with 3-arity conform and add implementations for it on all current specs. Currently this is not possible.

[1] https://github.com/metosin/spec-tools



 Comments   
Comment by Alex Miller [ 22/Feb/17 3:33 PM ]

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.

Comment by Tommi Reiman [ 23/Feb/17 1:26 AM ]

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?





[CLJ-2115] Support data conveying conform errors (alternative/complement to :clojure.spec/invalid) Created: 22/Feb/17  Updated: 22/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Max Penet Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec


 Description   

At the moment if a conform calls fails (returning :clojure.spec/invalid) there is no way to supply extra information about why it failed. We do have the possibility to get explain-data, but at best this would return a spec form, the original value and some metadata.

While this is fine in most cases, some conformer functions upon failure can provide extra data that'd be useful to the consumer. A practical example we had was a spec that can contain values for a String based DSL (think SQL like), that would conform these values to their parsed AST. When the conform wrapped function fails it would throw an ex-info with line/col info and more metadata about the failure. But all this data was lost since we can only return :clojure.spec/invalid. All this happened inside a rule engine schema, that can contain hundreds of these; re-parsing all the failing values for error reporting is something we wanted to avoid.

The proposal would be to to support a new return value that'd allow conveying data about the conform failure, or to support both this new value and :clojure.spec/invalid.
This could take the form of (explain-info {..}) potentially returned by conformer function for later consumption by explain, to match clojure semantics with exceptions (ex-info/ex-data, explain-info/explain-data).

A more naive implementation could just allow to throw inside the conformer function and have the error merged/assoced into the explain map (but that might be a bit too invasive in my opinion).






[CLJ-1496] Added a new arity to 'ex-info' that only accepts a message. Created: 08/Aug/14  Updated: 21/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: try-catch
Environment:

java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)

Mac OSX 10.9.4


Attachments: File ex_info_arity.diff    
Patch: Code
Approval: Triaged

 Description   

We often use 'ex-info' to throw a custom exception.But ex-info at least accepts two arguments: a string message and a data map.
In most cases,but we don't need to throw a exception that taken a data map.
So i think we can add a new arity to ex-info:

(ex-info "the exception message")

That created a ExceptionInfo instance carries empty data.

I am not sure it's useful for other people,but it's really useful for our developers.

The patch is attached.



 Comments   
Comment by Alex Miller [ 10/Feb/17 8:46 AM ]

Why "(. clojure.lang.PersistentArrayMap EMPTY)" ? Why not just {}?

Comment by Leon Grapenthin [ 14/Feb/17 1:43 PM ]

I always thought the lack of a one-arity was intentional design to make users use the map argument. Why do you want to throw ExceptionInfos with no data?

Comment by dennis zhuang [ 14/Feb/17 9:53 PM ]

@Alex I forgot why i used EMPTY map here, maybe influenced by the code https://github.com/clojure/clojure/blob/7aad2f7dbb3a66019e5cef3726d52d721e9c60df/src/clj/clojure/core.clj#L4336

@Leon For example, throw an exception when arguments error:

(when-not (integer? c)
(throw (ex-info "Expect number for c."))

We don't need data here.

Comment by Nicola Mometto [ 15/Feb/17 4:27 AM ]

Then why not just throw an `(Exception. "expect number for c")`? I don't see the added value in throwing exinfos w/o data vs just throwing an Exception

Comment by dennis zhuang [ 21/Feb/17 9:13 PM ]

Indeed, it was just a technical decision that we chose to use ex-info for throwing exceptions.





[CLJ-2103] s/coll-of and s/every take unnecessary long to generate if :into not provided Created: 28/Jan/17  Updated: 17/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: generator, spec
Environment:

alpha14



 Description   
(s/def ::bar (s/coll-of number? :kind vector?))

(defn foo [a]
  (reduce + 0 a))

(s/fdef foo
  :args (s/cat :a ::bar)
  :ret number?)

(first (stest/check `foo))

Doesn't terminate in a reasonable amount of time.
One observes that changing :gen-max doesn't affect computation time.

One observes that it can be fixed by adding :into [] to the s/coll-of spec.

The reason is this: If :into is not provided, s/every and s/coll-of has to generate a vector (via gen of vector?) to call empty on it, to then fill it up. This is quite clearly documented in the docstring of `s/every`:

:kind - a pred/spec that the collection type must satisfy, e.g. vector?
         (default nil) Note that if :kind is specified and :into is
         not, this pred must generate in order for every to generate.

Assumedly the vector? generates quite large vectors at a certain point which significantly slows down the generation.

The responsible code is in gen* of every-impl

(gen/bind
 (cond
  gen-into (gen/return (empty gen-into))
  kind (gen/fmap #(if (empty? %) % (empty %))
                 (gensub kind overrides path rmap form))  ;; creating and mapping gen of :kind
 :else (gen/return []))
 (fn [init]
    ....


 Comments   
Comment by Leon Grapenthin [ 14/Feb/17 3:03 PM ]

I see two approaches to improve this behavior:

1. The gen uses the gen of kind to generate one value with the smallest size, calls empty on it to determine :into. This would lead to a surprise when your :kind is e. g. (s/or :a-vec vector? :a-list list?) (which currently throws, anyway)
2. We use an internal lookup table to assume :into. {clojure.core/vector? [], clojure.core/set? #{} ...}





[CLJ-2114] ::defn-args spec incorrectly parses map body as a prepost rather than function body Created: 16/Feb/17  Updated: 16/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File 0001-CLJ-2144-conform-map-fn-bodies-as-body-rather-than-a.patch    
Patch: Code
Approval: Screened

 Description   

Reported by Claire Alvis in #clojure-spec:

user> (s/conform :clojure.core.specs/defn-args
                 '(foo [bar] {:baz 42}))
{:name foo, :bs [:arity-1 {:args {:args [[:sym bar]]}, :prepost {:baz 42}}]}

The current spec conforms function bodies with single maps as prepost conditions rather than function bodies, after the patch:

user=> (s/conform :clojure.core.specs/defn-args '(foo [bar] {:baz 42}))
{:name foo, :bs [:arity-1 {:args {:args [[:sym bar]]}, :body [:body [{:baz 42}]]}]}
user=> (s/conform :clojure.core.specs/defn-args '(foo [bar] {:baz 42} 1))
{:name foo, :bs [:arity-1 {:args {:args [[:sym bar]]}, :body [:prepost+body {:prepost {:baz 42}, :body [1]}]}]}

Patch: 0001-CLJ-2144-conform-map-fn-bodies-as-body-rather-than-a.patch

Screened by: Alex Miller



 Comments   
Comment by Nicola Mometto [ 16/Feb/17 5:42 PM ]

An open question is whether we also want to make `:prepost` stricter as part of this patch, so that it will ensure that `:pre` and `:post` are a collection





[CLJ-2113] Update Clojure maven for latest on CI server Created: 16/Feb/17  Updated: 16/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.9

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: build

Attachments: Text File mvn3.patch    
Patch: Code
Approval: Vetted

 Description   

Update maven build infrastructure to stop using oss-parent and use updated Maven 3 and sonatype plugins (similar to other changes made recently in all contrib projects).

  • Removed oss-parent parent pom. This has been deprecated for years and is no longer recommended for use.
  • Add snapshot repo (was previously pulled in via oss-parent)
  • maven-compiler-plugin - update to latest version
  • maven-release-plugin - update to latest version
  • add nexus-staging-maven-plugin - current recommended plugin for releases to maven central, replaces most of the maven-release-plugin's work (old version of this previously in oss-parent)
  • add maven-gpg-plugin for signing (previously in oss-parent)
  • remove old release profile which was activated by oss-parent pom

Patch: mvn3.patch

It's difficult to test this completely outside the context of actually building and deploying snapshots and releases but the changes are very similar to those made for all contrib projects recently.






[CLJ-1138] data-reader returning nil causes exception Created: 22/Dec/12  Updated: 15/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5, Release 1.6, Release 1.7, Release 1.8
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: reader
Environment:

clojure 1.5 beta2, Mac OS X 10.8.2, java version "1.6.0_37"


Attachments: Text File 0001-CLJ-1139-allow-nil-in-data-reader.patch     Text File clj-1139-2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

If a data-reader returns nil, the reader throws java.lang.RuntimeException: No dispatch macro... The error message implies that there is no dispatch macro for whatever the first character of the tag happens to be.

Here's a simple example:

user=> (binding [*data-readers* {'f/ignore (constantly nil)}] 
         (read-string "#f/ignore 42 10"))
RuntimeException No dispatch macro for: f  clojure.lang.Util.runtimeException (Util.java:219)

The original reader code did not distinguish between the absence of a data-reader and a returned value of nil from the appropriate data-reader. It therefore got confused and tried to find a dispatch macro, sending it further down the incorrect code path, ultimately yielding a misleading error message.

The original documentation did not distinguish nil as an illegal value. Clearly this bug was an oversight in the original data-reader code, not an intentional feature.

The patch uses a sentinel value to distinguish the missing data-reader case from the nil returned value case.

Patch: clj-1139-2.patch



 Comments   
Comment by Steve Miner [ 22/Dec/12 9:43 AM ]

clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch allows a data-reader to return nil instead of throwing. Does sanity check that possible tag or record isJavaIdentifierStart(). Gives better error message for special characters that might actually be dispatch macros (rather than assuming it's a tagged literal).

Comment by Steve Miner [ 22/Dec/12 10:06 AM ]

clj-1138-data-reader-return-nil-for-no-op.patch allows a data-reader returning nil to be treated as a no-op by the reader (like #_). nil is not normally a useful value (actually it causes an exception in Clojure 1.4 through 1.5 beta2) for a data-reader to return. With this patch, one could get something like a conditional feature reader using data-readers.

Comment by Steve Miner [ 22/Dec/12 10:26 AM ]

clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch is the first patch to consider. It merely allows nil as a value from a data-reader and returns nil as the final value. I think it does what was originally intended for dispatch macros, and gives a better error message in many cases (mostly typos).

The second patch, clj-1138-data-reader-return-nil-for-no-op.patch, depends on the other being applied first. It takes an extra step to treat a nil value returned from a data-reader as a no-op for the reader (like #_).

Comment by Steve Miner [ 23/Dec/12 11:52 AM ]

It turns out that you can work around the original problem by having your data-reader return '(quote nil) instead of plain nil. That expression conveniently evaluates to nil so you can get a nil if necessary. This also works after applying the patches so there's still a way to return nil if you really want it.

(binding [*data-readers* {'x/nil (constantly '(quote nil))}] (read-string "#x/nil 42"))
;=> (quote nil)

Comment by Andy Fingerhut [ 07/Feb/13 9:20 AM ]

Patch clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch dated Dec 22 2012 still applies cleanly to latest master if you use the following command:

% git am --keep-cr -s --ignore-whitespace < clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch

Without the --ignore-whitespace option, the patch fails only because some whitespace was changed in Clojure master recently.

Comment by Andy Fingerhut [ 13/Feb/13 11:24 AM ]

OK, now with latest master (1.5.0-RC15 at this time), patch clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch no longer applies cleanly, not even using --ignore-whitespace in the 'git am' command given above. Steve, if you could see what needs to be updated, that would be great. Using the patch command as suggested in the "Updating stale patches" section of http://dev.clojure.org/display/design/JIRA+workflow wasn't enough, so it should probably be carefully examined by hand to see what needs updating.

Comment by Steve Miner [ 14/Feb/13 12:21 PM ]

I removed my patches. Things have changes recently with the LispReader and new EdnReader.

Comment by Alex Miller [ 15/Feb/17 9:18 AM ]

Fixed whitespace warning and updated patch so it applies, no semantic changes, attribution retained in clj-1139-2.patch.

Comment by Alex Miller [ 15/Feb/17 9:27 AM ]

Ticket needs better description of problem and approach taken in the patch.

Comment by Steve Miner [ 15/Feb/17 10:09 AM ]

If the problem isn't clear, I would ask why would a nil return value be treated specially for a data-reader? And if it is considered illegal by design, does this error message enlighten the user?

I could not find any documented restriction at the time the bug was filed and I still can't find any today. So it seems like a simple bug to me. The data-reader should be allowed to return nil, and the Clojure reader should process the nil as usual. My work-around was to return (quote nil) which gave the intended behavior without triggering the bug.

Comment by Alex Miller [ 15/Feb/17 10:55 AM ]

Would appreciate more updates to the description. My question would be whether invoking a data reader function should ever return nil. Is there a good use case to need this? It seems you are reading the description of a non-nil tagged value with the reader and thus getting back nil is confusing. That's not possibly round-trippable and thus seems asymmetric.

Comment by Steve Miner [ 15/Feb/17 1:34 PM ]

Nulla poena sine lege or basically unless you say it's illegal, I should be able to do it. My trivial example is #C NULL which seems like an obvious nil to me.

Looking back on this issue, I can see that most people think of tagged literals as a way of encoding foreign values in Clojure literals. If you only care about an extensible data notation, who needs another way of writing nil? That's a fair question.

I wanted to use data-readers as somewhat circumscribed reader macros (as used in Common Lisp). I discovered this bug while I was doing something platform specific (long before reader conditionals were implemented). In my situation, it was convenient to return nil on "other" platforms.

Many usages of data-readers are not bijective. For example, #infix (3 + 4) interpreted as constant 7 is likewise not round-trippable. Unless you're Dan Friedman or Wil Byrd, round-tripping is a tough requirement.

I will try to update my description with a bit more context, but I don't want to distract anyone from the obvious bug (and bad error message) with my unorthodox usage.

Comment by Steve Miner [ 15/Feb/17 1:45 PM ]

By the way, this bug is CLJ-1138, but the proposed patch says "1139" which might confuse some busy reviewers.

Comment by Steve Miner [ 15/Feb/17 2:13 PM ]

I tested the patch and it worked well for me with the current master. I would suggest adding another test to confirm that the edn/read-string works correctly as well. Here's what I used. This also tests that overriding the default readers works. Please feel free to take the test if you want it.

(deftest clj-1138-uuid-override
  (is (nil? (binding [*data-readers* {'uuid (constantly nil)}]
              (read-string "#uuid \"550e8400-e29b-41d4-a716-446655440000\""))))
  (is (nil? (edn/read-string {:readers {'uuid (constantly nil)}}
                             "#uuid \"550e8400-e29b-41d4-a716-446655440000\""))))




[CLJ-2112] Add specs for spec forms Created: 15/Feb/17  Updated: 15/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 7
Labels: spec

Attachments: Text File spec-forms.patch    
Patch: Code
Approval: Incomplete

 Description   

It would be useful to have specs that described spec forms, such that it was possible to go from a spec form like (s/keys :req [::a ::b] :opt [::c]) to a conformed version that allowed you to grab the parts without parsing the s-expression. This can be done by creating specs, thus allowing:

user=> (require '[clojure.spec :as s] '[clojure.spec.specs])
user=> (s/def ::aspec (s/keys :req [::a ::b] :opt [::c]))
user=> (def aspec-data (s/conform :clojure.spec.specs/spec (s/form ::aspec)))
user=> (pr aspec-data)
[:form {:s clojure.spec/keys, 
        :args {:req [[:key :clojure.spec.specs/a] [:key :clojure.spec.specs/b]], 
               :opt [:clojure.spec.specs/c]}}]
user=> (map val (-> aspec-data val :args :req))
(:clojure.spec.specs/a :clojure.spec.specs/b)

Patch: spec-forms.patch (a work in progress)






[CLJ-2040] Allow runtime modification of REPL exception handling Created: 11/Oct/16  Updated: 15/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: repl

Attachments: Text File CLJ-2040-dynamic-repl-exceptions.patch    
Patch: Code
Approval: Triaged

 Description   

Problem Statement

Clojure's REPL is capable of paramterizing almost every aspect of its functionality, including how uncaught exceptions are printed. In the current implementation, these customization hooks are passed in as arguments and closed over, meaning that they cannot be changed once the REPL is started.

Many development tools want to override how the REPL handles uncaught errors. Examples of useful customizations include (but are not limited to):

  • Formatted exception messages (including whitespace and ANSI coloring)
  • Alternative representations for certain types of exceptions (e.g, Spec errors)
  • Dropping into a graphical interaction mode to better inspect ex-data.

Currently, this type of customization must be applied before a REPL is started, meaning that changing how a REPL displays errors requires support from (or plugins to) a third-party tool such as Boot or Leiningen.

Alternatives


1. Take no action.

Third-party tool support is required to create customized exception handling in the REPL. Tools have different techniques for doing this:

  • nREPL can intercept the exception on the wire and passes it through middleware
  • Leiningen plugins alter the root binding of clojure.main/repl-caught.
  • Boot allows users to build a task to invoke clojure.main/repl with the desired arguments.

Users will continue to select one of these according to their tooling preferences.

Benefits:
1. No effort or changes to the existing code.

Tradeoffs:
1. Tools will continue to implement their own diverse, sometimes hacky techniques for printing custom exceptions.
2. Any library intended to provide alternative exception handling will be tied to a specific launcher tool.

2. Make the REPL exception handler dynamically rebindable

If the REPL exception handler were a dynamic, thread-local var, users and libraries could change the behavior of the currently running REPL.

Benefits:
1. Users and libraries can freely override how exceptions are printed, regardless of how Clojure was launched.
2. Fully backwards compatible with existing tools.

Tradeoffs:
1. It will be possible for library authors to provide "bad" or poorly reasoned error printers. This is still possible with launch tools, but the barrier of entry is even lower with libraries.

The attached patch implements this option.

3. Encourage users to start new REPLs instead

In many Clojure environments, it's possible to explicitly launch a REPL from within another REPL. This sub-REPL could have the desired :caught hook.

Benefits:
1. No effort or changes to the existing code.
2. "Functionally pure", and in alignment with the evident design of the current REPL.

Tradeoffs:
1. There is a non-trivial subset of Clojure developers who do not know exactly how REPLs work. They are likely to be confused or subject to increased cognitive load. Insofar as this set of beginner/intermediate developers are precisely who enhanced error messages are meant to help in the first place, this solution is counterproductive.
2. For better or for worse, many existing and widely used tools do not support this. This does not work at all in nREPL, for example. However, even the simplest command-line REPLs behavior would change for the worse; sending a EOF (accidentally or otherwise) would always kill the sub-REPL with no feedback as to what just happened.



 Comments   
Comment by Alex Miller [ 15/Feb/17 9:39 AM ]

On the repl-caught var, it would be good to mention the signature of the handler, namely that it takes an exception, is expected to print or otherwise handle the exception, and that its return will be ignored.

Rather than the changes in repl, what if you added a dynamic-repl-caught and change that to be the default caught handler in repl? Or even just changed repl-caught itself.





[CLJ-2111] s/every :kind does not work as intended Created: 14/Feb/17  Updated: 14/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Vetted

 Description   
user=> (s/valid? (s/every number? :kind (s/or :vector vector? :list list?))
          [])
ClassCastException clojure.spec$or_spec_impl$reify__13891 cannot be cast to clojure.lang.IFn  dev/eval44499/fn--44501 (form-init3178965928127409998.clj:22)

user=> (pst *e)
ClassCastException clojure.spec$or_spec_impl$reify__13903 cannot be cast to clojure.lang.IFn
	user/eval20/fn--22 (NO_SOURCE_FILE:13)
	clojure.spec/every-impl/reify--14039 (spec.clj:1225)
	clojure.spec/valid? (spec.clj:744)
	clojure.spec/valid? (spec.clj:740)

Expected: true



 Comments   
Comment by Alex Miller [ 14/Feb/17 5:06 PM ]

Certainly a function like this works (s/every number? :kind #(or (vector? %) (list? %))). The question is whether the s/every doc that states "pred/spec" means only a predicate function or "predicate function OR spec". I'm not sure what the intention was. Certainly the code in every seems to be wrapping the kind into a function and then invoking it in every-impl, so it's not written to accept a spec currently.

Comment by Alex Miller [ 14/Feb/17 5:06 PM ]

Marking vetted to either resolve, update docstring, or decline. Need more info from Rich.





[CLJ-2066] Reflection on internal classes fails under Java 9 Created: 22/Nov/16  Updated: 14/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: interop, reflection

Attachments: Text File tcrawley.CLJ-2066.2017-02-14.patch    
Approval: Triaged

 Description   

With the module system (jigsaw) as it is currently implemented in Java 9 early access builds (9-ea+144), calling a method via reflection is only allowed if the Method was retrieved for a class/interface in a package that is exported by its containing module. Reflector.java currently uses only target.getClass() to locate the Method, so reflective method invocation on a non-exported class will fail even if the method is provided by an exported parent interface or superclass.

The current workaround is to export the package to the unnamed module (where an application that doesn't explicitly use the module system runs) when invoking java/javac:

java --add-exports=java.xml/com.sun.xml.internal.stream=ALL-UNNAMED --add-exports=java.xml/com.sun.xml.internal.stream.writers=ALL-UNNAMED --add-exports=java.xml/com.sun.org.apache.xerces.internal.impl=ALL-UNNAMED ...

It's possible that this will be addressed in jigsaw before the release of Java 9. If not, Reflector.java could be modified to walk the ancestor chain if the initial invocation fails. Note that even with that change, accessing methods that are only defined on the non-exported class (i.e. methods that don't override a method from an exported superclass/interface) will require an --add-exports option.

For more details, see https://groups.google.com/d/msg/clojure-dev/Tp_WuEEcdWI/LMMQVAUYBwAJ



 Comments   
Comment by Toby Crawley [ 22/Nov/16 10:02 AM ]

This is the root cause of http://dev.clojure.org/jira/browse/DXML-32

Comment by Toby Crawley [ 14/Feb/17 3:48 PM ]

I don't see any indication that Java 9 will change in our favor, so I'm attaching a patch to fix this in Clojure itself.





[CLJ-2110] sorted map returns nil value for existing key Created: 14/Feb/17  Updated: 14/Feb/17  Resolved: 14/Feb/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8
Fix Version/s: None

Type: Defect Priority: Major
Reporter: jonnik Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: sorted-map
Environment:

Oracle Java 1.8.0_112



 Description   

Then i try to get value by key from sorted map with comparator by value it returns nil.

(def tmap {1 {:v 1} 2 {:v 2} 3 {:v 3}})
(def tmap-sorted (apply sorted-map-by #(let [val-comp (- (compare (get-in tmap [%1 :v]) (get-in tmap [%2 :v])))]
                                         (if (= val-comp 0) 1 val-comp))
                        (flatten (vec tmap))))
; => {3 {:v 3} 2 {:v 2} 1 {:v 1}}
(get tmap-sorted 3)
;=> nil

Expected: {:v 3}
Actual: nil



 Comments   
Comment by Andy Fingerhut [ 14/Feb/17 11:31 AM ]

Your comparison function never returns 0, by the way you have written it. If it never returns 0, it will never 'think' that it has found an equal element when searching for a match using 'get'. By replacing (if (= val-comp 0) 1 val-comp) with val-comp, the get calls will work:

(def tmap-sorted (apply sorted-map-by #(let [val-comp (- (compare (get-in tmap [%1 :v]) (get-in tmap [%2 :v])))]
                                         val-comp)
                        (flatten (vec tmap))))
tmap-sorted
; => {3 {:v 3}, 2 {:v 2}, 1 {:v 1}}
(get tmap-sorted 3)
; => {:v 3}

You are getting a descending sorted order by negating the return value of compare. I would recommend to follow the advice on this page: https://clojure.org/guides/comparators particularly "Reverse the sort by reversing the order that you give the arguments to an existing comparator." That helps avoid corner cases with some integer values.

I would also recommend (into my-empty-sorted-map tmap) in place of your (apply my-empty-sorted-map (flatten (vec tmap)). Putting all of those recommendations together would result in code like this:

(def tmap-sorted2 (into (sorted-map-by #(compare (get-in tmap [%2 :v]) (get-in tmap [%1 :v])))
                        tmap))
tmap-sorted2
; => {3 {:v 3}, 2 {:v 2}, 1 {:v 1}}
(get tmap-sorted2 3)
; => {:v 3}
Comment by saintech [ 14/Feb/17 11:41 AM ]

Ok. But how about this?:

tmap-sorted
; => {3 {:v 3} 2 {:v 2} 1 {:v 1}}
(first tmap-sorted)
; => [3 {:v 3}]
(get tmap-sorted 3)
;=> nil

Is it OK?

Comment by Andy Fingerhut [ 14/Feb/17 12:43 PM ]

I believe that calling clojure.core/first on a sorted-map does not cause its comparison function to be called at all. It is already stored in a sorted tree in memory, and first just finds the earliest one.

clojure.core/get does call the comparison function, perhaps several times, to find an item with an equal key. The original comparison function given in the description never returns equality (i.e. the integer 0) when comparing two items.

Comment by Alex Miller [ 14/Feb/17 1:32 PM ]

Agreed with Andy, declining





[CLJ-2108] Loading core specs affects startup time Created: 13/Feb/17  Updated: 13/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Vetted

 Description   

Adding the loading of spec itself + the clojure.core.specs namespace containing specs for core makes start time worse (and will only get longer as we add more core specs).



 Comments   
Comment by Ghadi Shayban [ 13/Feb/17 4:58 PM ]

Locally,
alpha14 takes 1.02s

time java -jar clojure-1.9.0-alpha14.jar -e ':foo'

Master with this line commented out takes 0.92s

time java -jar target/clojure-1.9.0-master-SNAPSHOT.jar -e ':foo'




[CLJ-2109] Protocol methods not instrumented Created: 13/Feb/17  Updated: 13/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec
Environment:

Clojure 1.9.0-alpha14


Approval: Triaged

 Description   

Spec instrument does not work on protocol methods. Invalid arguments will be accepted silently with no error. Protocol vars are included in the return value of (instrument).

Steps to reproduce

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

(defprotocol P
  (method [this arg]))

(defrecord R []
  P
  (method [this arg]
    (str "R.method called with " (pr-str arg))))

(s/fdef method
  :args (s/cat :this any?
               :arg number?))

(defn wrapped [this arg]
  (method this arg))

(s/fdef wrapped
  :args (s/cat :this any?
               :arg number?))

(test/instrument)

(println (method (->R) "not a number"))

(println (wrapped (->R) "not a number"))

This code produces the output:

R.method called with "not a number"
clojure.lang.ExceptionInfo: Call to #'user/wrapped did not conform to spec:
In: [1] val: "not a number" fails at: [:args :arg] predicate: number?
...

Possible resolutions

1. Add support to instrument for protocol methods
2. Document that instrument does not work on protocol methods, do not return protocol method Vars from (instrument), throw exception if protocol method Vars are included in the symbols passed to (instrument syms)

See also

CLJ-1941 describes a different case where instrument does not work. This issue was identified in a comment.

Workarounds

This issue can be avoided by wrapping protocol methods in normal functions and spec'ing the functions. This is already common practice.






[CLJ-1776] Test that collections are valid statements Created: 08/Jul/15  Updated: 09/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Michael Blume Assignee: Michael Blume
Resolution: Unresolved Votes: 0
Labels: compiler, test

Attachments: Text File clj-1776-v1.patch     Text File CLJ-1776-v2.patch    
Patch: Code and Test

 Description   

It's possible to break the compiler such that vectors are emitted incorrectly when they're in statement position (I accidentally did this). This doesn't break any part of the Clojure test suite, but does break valid Clojure code (for me it hit taoensso's encore). Add tests to the test suite so defects of this kind are caught.



 Comments   
Comment by Daniel Compton [ 09/Feb/17 8:34 PM ]

Is there a compiler change required as well here?

Comment by Alex Miller [ 09/Feb/17 10:56 PM ]

I think he is saying that the compiler is fine but that if it weren't, no test would tell you that.





[CLJ-2106] satisfies? is quite slow when returning false Created: 06/Feb/17  Updated: 09/Feb/17  Resolved: 09/Feb/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Mike Kaplinskiy Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: performance


 Description   
(defprotocol SatisfiesTest (testThing [this]))
(defrecord MyRecord [] Object SatisfiesTest (testThing [this]))
(def r (MyRecord.))

(time (dotimes [_ 30000] (instance? user.SatisfiesTest r)))
"Elapsed time: 1.715 msecs"

(time (dotimes [_ 30000] (satisfies? SatisfiesTest r)))
"Elapsed time: 3.944 msecs"

(time (dotimes [_ 30000] (instance? user.SatisfiesTest {})))
"Elapsed time: 2.304 msecs"

(time (dotimes [_ 30000] (satisfies? SatisfiesTest {})))
"Elapsed time: 718.949 msecs"

It would be nice if satisfies? memoized negative return values by class (though that cache would need to be cleared by `extend-type` and friends).



 Comments   
Comment by Alex Miller [ 06/Feb/17 1:56 PM ]

This is covered in an existing ticket - http://dev.clojure.org/jira/browse/CLJ-1814

Comment by Alex Miller [ 06/Feb/17 2:02 PM ]

Actually, read too quickly - CLJ-1814 covers the positive case only. I don't think we're going to cache a negative return value though. Managing and cleaning that cache is likely to not be worth it. If you need this kind of thing, you should probably consider a different kind of conditional check before-hand.

Comment by Alex Miller [ 06/Feb/17 2:05 PM ]

For example, you can just memoize this call like this:

user=> (def check (memoize #(satisfies? SatisfiesTest %)))
#'user/check
user=> (time (dotimes [_ 30000] (check {})))
"Elapsed time: 3.512 msecs"
Comment by Alex Miller [ 06/Feb/17 2:13 PM ]

I tweaked the test to remove the use of range and the construction of the record so the numbers are more useful.

Comment by Nicola Mometto [ 08/Feb/17 6:52 PM ]

Alex Miller CLJ-1814 handles the negative cases too, and doesn't keep a cache of negative return values. It keeps a cache of all the protocols extended by a certain class and invalidates that cache on every call to extend, the benchmarks on the ticket description showcase both a positive and a negative test

Comment by Alex Miller [ 09/Feb/17 3:09 PM ]

Nicola - cool! I didn't realize that. Will mark this as a dupe then.





[CLJ-2107] s/explain of a predicate defined s/with-gen yields s/unknown Created: 07/Feb/17  Updated: 08/Feb/17  Resolved: 08/Feb/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Tamas Herman Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: generator, spec


 Description   

This explanation is clear:

(s/def ::some-string string?)
(s/explain ::some-string 123)

val: 123 fails spec: :boot.user/some-string predicate: string?

However if the failing spec has a custom generator, the explanation is not so helpful:

(s/def ::some-string-with-custom-generator (s/with-gen string? gen/string-alphanumeric))
(s/explain ::some-string-with-custom-generator nil)

=> :boot.user/some-string-with-custom-generator
val: nil fails spec: :boot.user/some-string-with-custom-generator predicate: :clojure.spec/unknown

I would expect the same predicate: string? explanation.

Based on the symptom I suspect this issue is related to http://dev.clojure.org/jira/browse/CLJ-2068



 Comments   
Comment by Tamas Herman [ 08/Feb/17 2:21 AM ]

Explanation of a workaround from Slack from @hiredman:

with-gen is a function, so it's arguments are evaluated, so the string? argument to with-gen is a function object, when spec is trying to find the name to report it does some stuff, which for symbols and keywords reports a good name, but for other Objects (including function objects) you get :clojure.spec/unknown.

wrapping with s/spec allows the spec macro to capture the meaningful, the symbol before evaluation:

(s/def ::some-string-with-custom-generator (s/with-gen (s/spec string?) gen/string-alphanumeric))
(s/explain ::some-string-with-custom-generator nil)

val: nil fails spec: :boot.user/some-string-with-custom-generator predicate: string?

I don't see any simple solution to make the original example yield a good explanation,
so maybe we should just explain how it works in https://clojure.org/guides/spec#_custom_generators
and wrap the with-gen examples with spec.

Comment by Alex Miller [ 08/Feb/17 8:49 AM ]

As you mentioned, this is a dup of CLJ-2068 and is fixed by the patch there.





[CLJ-2105] incorrect spec conform when an optional (?) is inside of a one or more (+) Created: 03/Feb/17  Updated: 03/Feb/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Anthony D'Ambrosio Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

1.9.0-alpha14


Approval: Vetted

 Description   
(s/def ::thing (s/cat :a (s/? string?) :b (s/+ number?))) 
(s/def ::seq-of (s/+ ::thing))

(s/conform ::seq-of (list "foo" 1 "bar" 2 3 "qux" 4)) 
;=> [{:a "foo", :b [1]} [{:a "bar", :b [2 3]} {:a "qux", :b [4]}]]

;EXPECTED 
;=> [{:a "foo", :b [1]} {:a "bar", :b [2 3]} {:a "qux", :b [4]}]

;; ONLY 2nd thing matters? 
(s/conform ::seq-of (list "foo" 1 2 "bar" 3)) 
;=> [{:a "foo", :b [1 2]} {:a "bar", :b [2]}]

;; NO OPTIONAL 
(s/def ::thing (s/cat :a string? :b (s/+ number?))) 
(s/def ::seq-of (s/+ ::thing))
(s/conform ::seq-of (list "foo" 1 "bar" 2 3 "qux" 4)) 
;=> [{:a "foo", :b [1]} {:a "bar", :b [2 3]} {:a "qux", :b [4]}]

This also only shows up if there are 2+ numbers in the 2nd or later ::thing
AND the problem goes away if I make :a not optional...

Could be related to http://dev.clojure.org/jira/browse/CLJ-2003 ?






[CLJ-2080] clojure.spec/every-kv does not work correctly on vectors Created: 08/Dec/16  Updated: 31/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Defect Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

alpha 14


Attachments: Text File clj-2080-2.patch     Text File clj-2080-3.patch     Text File clj-2080.patch    
Patch: Code
Approval: Vetted

 Description   

The every-kv doc states "takes separate key and val preds and works on associative collections". Vectors return true for associative? but do not currently work:

user=> (s/conform (s/every-kv any? any?) [])
[]
user=> (s/conform (s/every-kv any? any?) [1 2 3])
:clojure.spec/invalid
user=> (s/conform (s/every-kv integer? string?) [])
[]
user=> (s/conform (s/every-kv integer? string?) ["x"])
:clojure.spec/invalid

Another similar problem:

(s/explain-data (s/every-kv int? int?) [{:a :b}])
UnsupportedOperationException nth not supported on this type: PersistentArrayMap  clojure.lang.RT.nthFrom (RT.java:903)

Cause: The combination of every-kv and every-impl assume that the collection passed to every-kv can provide a seq of map entries (which is the intention). In the explain case, the ::kfn created by every-kv is used to create a better path segment using the key rather than the element index. The kfn assumes that an element of the collection can call `(nth entry 0)` on the element. In the explain failure above, the map {:a :b} will throw when invoked with nth.

Proposed: Do the following to be clearer about the requirement that the coll elements are map entries:

  • Modify the docstring to say "seqs to map entries" rather than "is a map"
  • Modify the kfn to add a check that the element is an entry and if so, use it's key. If not, use the index (of the element itself). In this case, when passed an entry it will report with the actual key but when passed something that's not an entry, it will report the collection based index of the non-entry.

Patch: clj-2080-3.patch



 Comments   
Comment by Alex Miller [ 09/Dec/16 8:35 AM ]

At the moment, I'm inclined to say the doc in every-kv should be tightened to say "map" instead of "associative collection" but will check with Rich.





[CLJ-2056] Efficient shortcut for (first (filter pred coll)) idiom Created: 11/Nov/16  Updated: 30/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 27
Labels: None

Attachments: Text File clj-2056-clojure-core-seek-2.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

It's a common task to look up for an item in a collection based on predicate. Currently Clojure has no direct support for that in clojure.core. Instead, our options are:

1. (first (filter pred coll)) will create intermediate lazy sequence and might evaluate pred up to 31 extra times in case of chunked sequence

2. (some #(when (pred %) %) coll) will short-circuit on first match, but won't catch false value in something like (some #(when false? %) [true false true])

Additionally, both of these workarounds a) obscure the purpose of the code, and b) do not handle custom not-found values.

Attached is a patch that makes use of efficiency of reduce-able collections, handles edge cases like looking for false? or nil?, and supports optional not-found value.

Examples:

(seek odd? (range)) => 1
(seek pos? [-1 1]) => 1
(seek pos? [-1 -2] ::not-found) => ::not-found
(seek nil? [1 2 nil 3] ::not-found) => nil

Patch: clj-2056-clojure-core-seek-2.patch

Prescreening notes: I think the general approach is good. Is it necessary to support nil? and false? preds? Or would a transduce formulation like the one in comments be sufficient.



 Comments   
Comment by Alex Miller [ 11/Nov/16 8:54 AM ]

Just as an interesting aside, the new halt-when transducer could actually be used to create something like this too (if you set aside the desire to support nil? and false? preds).

(transduce (comp (filter pred) (halt-when any?)) identity nil coll)

Patch has some trailing whitespace in the test code - could you clean that up?

Comment by Nikita Prokopov [ 12/Nov/16 3:46 AM ]

Attaching patch with trailing whitespace cleaned

Comment by Nikita Prokopov [ 12/Nov/16 3:46 AM ]

Thanks Alex! Attached new patch with whitespace cleaned

Comment by Moritz Heidkamp [ 30/Jan/17 12:34 PM ]

I had a similar train of thought today and arrived at the idea of adding transducer support to first, e.g. (first (filter pred) coll). This could potentially be as efficient as the special-purpose seek function proposed above but would of course not support the not-found value. However, it seems like a natural extension to first and could be useful for other purposes, too.





[CLJ-2104] Typo in pprint docstring Created: 29/Jan/17  Updated: 29/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, typo

Attachments: Text File clj-2104.patch    
Patch: Code
Approval: Prescreened

 Description   

Typo in pprint ns docstring: "complete documentation on the the clojure web site on github"






[CLJ-2025] When a generator fails to gen, state which spec/pred failed Created: 21/Sep/16  Updated: 29/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: David Collie Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec

Attachments: Text File better-such-that-info.patch    
Approval: Triaged

 Description   

Given a generator using such-that that fails to find a value, the error does not give enough information to determine which spec or predicate was at fault:

(require '[clojure.spec :as s])
(s/exercise (s/and string? #{"hi"}))
ExceptionInfo Couldn't satisfy such-that predicate after 100 tries.  clojure.core/ex-info (core.clj:4725)

Another special case of this is when providing a custom generator that produces a valid that doesn't satisfy the spec (Clojure adds this filter internally):

(require '[clojure.spec :as s])
(s/exercise (s/with-gen int? #(s/gen #{:a})))

Proposal: Indicate in the error which spec failed to generate and possibly the path in the overall spec if feasible.

(Note: original description moved to comment)



 Comments   
Comment by Alex Miller [ 22/Sep/16 3:54 PM ]

[Original description from ticket:]

I created a generator that did not conform to the spec (doh!). The generator contained the such-that predicate. When I tried creating a sample from the generator I got this error:

ExceptionInfo Couldn't satisfy such-that predicate after 100 tries.  clojure.core/ex-info (core.clj:4725)

I assumed that it referred to my custom generator but that was a red herring because in fact spec must be using such-that to ensure that the generated value conforms to the spec, and it was this such-that that generated the failure, not the one in my custom generator.

Code (with the problem corrected but showing the such-that in my generator:

(defn mod11-checkdigit
  "Calculate the checkdigit see http://freagra.com/imthealth/mitNNC.html"
  [n]
  (let [x (->> (map #(Integer/parseInt (str %)) (take 9 n))
               (map * (range 10 1 -1))
               (reduce +))
        y (mod x 11)
        c (- 11 y)]
    (cond (== 10 c) nil
          (== 11 c) 0
          :else c)))

(def nhs-number-gen
  "Generates a valid NHS number"
  (gen/fmap #(str (+ (* 10 %) (mod11-checkdigit (str %))))
            (gen/such-that #(mod11-checkdigit (str %))
                           (gen/choose 100000000 999999999))))

(defn nhs-number?
  "Returns true if passed a valid nhs number else returns false"
  [n]
  (and (string? n) (= 10 (count n)) (= (str (mod11-checkdigit n)) (str (last n)))))

(s/def ::nhs-number (s/with-gen nhs-number?
                                (fn [] nhs-number-gen)))

It would be nicer if the error thrown due to the generated value being non-conformant with the spec stated this.

Comment by Alex Miller [ 22/Sep/16 4:07 PM ]

I'm not sure that this is possible right now based on what we give to and get back from test.check.

Comment by Griffin Smith [ 29/Jan/17 6:37 PM ]

It looks like test.check is being updated to support error customization: https://github.com/clojure/test.check/commit/5aea0e275257680b672309b1e940be6dae92c17d . I've got a patch which updates clojure.spec to use it, though obviously it doesn't work since the linked commit hasn't made it into a released version of test.check yet.

Comment by Griffin Smith [ 29/Jan/17 6:40 PM ]

See also http://dev.clojure.org/jira/browse/TCHECK-107 I suppose

Comment by Griffin Smith [ 29/Jan/17 6:51 PM ]

Attached the patch for future reference (`better-such-that-info.patch`).





[CLJ-2021] case where spec/conform -> spec/unform -> spec/conform gives invalid result Created: 12/Sep/16  Updated: 28/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Jeroen van Dijk Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: spec
Environment:

clojure 1.9, mac osx, java 1.8



 Description   

The example belows shows a case where a conform-ed form, does not conform any after an unform. It would be my expectation that you can repeat conform -> unform -> conform endlessly and get the same result.

(require '[clojure.core.specs])
(require '[clojure.spec :as s])

(s/def ::defn-macro (s/cat :type #

Unknown macro: {'defn}
:definition :clojure.core.specs/defn-args))

(let [form '(defn foo "bar" ([a & b] a a c) ([a b] a))]

(-> form
(->> (s/conform ::defn-macro))) ;;=> {:type defn, :definition {:name foo, :docstring "bar", :bs [:arity-n {:bodies [{:args {:args [[:sym a]], :varargs {:amp &, :form [:sym b]}}, :body [a a c]} {:args {:args [[:sym a] [:sym b]]}, :body [a]}]}]}}

;; Unforming returns the function definition, but with the args in a list instead of a vector:
(->> form
(s/conform ::defn-macro)
(s/unform ::defn-macro)) ;;=> (defn foo "bar" ((a (& b)) a a c) ((a b) a)))

;; Conforming after unforming doesn't work anymore
(->> form
(s/conform ::defn-macro)
(s/unform ::defn-macro)
(s/conform ::defn-macro)) ;;=> :clojure.spec/invalid

)



 Comments   
Comment by Jeroen van Dijk [ 12/Sep/16 8:22 AM ]

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

Comment by Leon Grapenthin [ 28/Jan/17 4:49 PM ]

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.





[CLJ-2013] Alternative s/cat options not error-reported Created: 24/Aug/16  Updated: 28/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, spec
Environment:

alpha14


Attachments: Text File CLJ-2013.patch    
Approval: Triaged

 Description   

This problem was detected in context of this discussion https://groups.google.com/d/msg/clojure/mIlKaOiujlo/tF71zZ2BCwAJ

A minimal version of how specs error reporting failed the users intuition there looks like this:

He used an invalid ns form

(ns foo (require [clojure.spec :as s])) ; should be :require

The error reported by spec:

In: [1] val: ((require [clojure.spec :as s])) fails at: [:args] predicate: (cat :docstring (? string?) :attr-map (? map?) :clauses :clojure.core.specs/ns-clauses),  Extra input
:clojure.spec/args  (foo (require [clojure.spec :as s]))
  clojure.core/ex-info (core.clj:4725)

While the error is technically true, it doesn't show the user /how/ each of the alternative options of the reported s/cat failed.

To get a better understanding why the users data is not correct, he should know precisely what spec tried and how it failed.

A good example of how this works is s/alt, where all failing alternatives are always reported to the user.

The problem has been investigated, first experimentally, then in specs code. Finally, a patch that brings error reporting like s/alts comes attached.

It has been observed that specs error reporting behavior for cat with optional branches is the following:

1. If the cat failed after one or many optional branches, the entire cat is reported as failing
2. If the cat failed after one or many optional branches /and/ a subsequent required branch, only the subsequent required branch is reported with no remarks to the alternative optional branches.

Rule 1 explains the ns example.
Rule 2 can fail the users intuition significantly worse:

(s/explain (s/cat :maybe-num (s/? number?)
                  :keyword keyword?)
           ["3"])

gives

In: [0] val: "3" fails at: [:keyword] predicate: keyword?

The report clearly doesn't address the users intent of putting in a number. Instead he is made to believe that he should have entered a keyword.

Solution:

A simple patch has been programmed that changes op-explain to have the following behaviour:

  • All alternatives that have been tried in a s/cat are reported individually.

It improves the reported errors significantly because it makes clearly transparent how the users data failed the validation.

(ns foo (require [clojure.spec :as s])) ; should be :require

now gives

ExceptionInfo Call to clojure.core/ns did not conform to spec:
In: [1] val: (require [clojure.spec :as s]) fails at: [:args :docstring] predicate: string?
In: [1] val: (require [clojure.spec :as s]) fails at: [:args :attr-map] predicate: map?
In: [1 0] val: require fails spec: :clojure.core.specs/ns-refer-clojure at: [:args :clauses :refer-clojure :clause] predicate: #{:refer-clojure}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-require at: [:args :clauses :require :clause] predicate: #{:require}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-import at: [:args :clauses :import :clause] predicate: #{:import}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-use at: [:args :clauses :use :clause] predicate: #{:use}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-refer at: [:args :clauses :refer :clause] predicate: #{:refer}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-load at: [:args :clauses :load :clause] predicate: #{:load}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-gen-class at: [:args :clauses :gen-class :clause] predicate: #{:gen-class}
:clojure.spec/args  (foo (require [clojure.spec :as s]))
  clojure.core/ex-info (core.clj:4725)

It would be even better if explain-data sorted ::s/problems by length of their :path which would push the first two unintended options to the end.

(s/explain (s/cat :maybe-num (s/? number?)
                  :keyword keyword?)
           ["3"])

now gives

In: [0] val: "3" fails at: [:maybe-num] predicate: number?
In: [0] val: "3" fails at: [:keyword] predicate: keyword?

While examples can be made up where this reporting produces more noise (like defn) I believe its the right tradeoff for aforementioned reasons and:

  • We programmers always ask our users for the most specific information when something went wrong - It is correct to apply the same to specs error reporting
  • Custom error reporters (s/explain-out) get more data to generate narrow reports matching the users intent even better


 Comments   
Comment by Nuttanart Pornprasitsakul [ 12/Oct/16 8:43 AM ]

I'll put a somewhat different issue here because I think it has the same root cause.

It should specifically say :ret of fspec is missing but it says failing at :args.

(require '[clojure.spec :as s])
(require '[clojure.spec.test :as st])

(defn x [f] (f 1))

(s/fdef x
  :args (s/cat :f (s/fspec :args (s/cat :i int?))))

(st/instrument `x)

(x (fn [a] a))
Exception in thread "main" clojure.lang.ExceptionInfo: Call to #'user/x did not conform to spec:
In: [0] val: (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"]) fails at: [:args] predicate: (cat :f (fspec :args (cat :i int?))),  Extra input
:clojure.spec/args  (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"])
:clojure.spec/failure  :instrument
:clojure.spec.test/caller  {:file "debug.clj", :line 16, :var-scope user/eval20}
 {:clojure.spec/problems [{:path [:args], :reason "Extra input", :pred (cat :f (fspec :args (cat :i int?))), :val (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"]), :via [], :in [0]}], :clojure.spec/args (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"]), :clojure.spec/failure :instrument,
...




[CLJ-2099] Keywords with aliased namespaces cannot be read when the namespace is required in a reader conditional Created: 13/Jan/17  Updated: 25/Jan/17  Resolved: 13/Jan/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Viktor Magyari Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: keywords, reader, readerconditionals


 Description   

The title in itself isn't entirely true, but I couldn't find a way to describe it succintly (feel free to change).

The issue is easier to demonstrate with an example:

(ns foo
  #?(:cljs (:require [clojure.core :as c])))

#?(:cljs ::c/x)

When reading this in a :clj context, the reader cannot read ::c/x ("Invalid token: ::c/x"), despite the code being correct (presumably).
The same thing happens if the reader conditional branches are :clj and the source is read in a :cljs context.



 Comments   
Comment by Alex Miller [ 13/Jan/17 8:07 AM ]

This looks like expected behavior to me. Auto-resolved keywords rely on a resolution context and there isn't one at the point where ::c/x is being read.

Comment by Viktor Magyari [ 13/Jan/17 9:05 AM ]

To me it seems reasonable to expect the resolution context to include the clojure.core alias - more generally, include <platform> specific aliases in the <platform> branches of reader conditionals. Maybe consider this as an enhancement ticket?

Comment by Alex Miller [ 13/Jan/17 9:18 AM ]

To do this would require adding special handling specifically for ns or other things that create aliases, which implies conditional evaluation of some forms at read-time. You would also need some place (other than the current platform's namespace maps) to track other platform's namespace aliases. That's a lot of very special, custom stuff.

We're not going to add this.

Comment by Leon Grapenthin [ 25/Jan/17 2:13 PM ]

1. Why does the reader try to read the :cljs branch in Viktors example?
2. The original intent of reader conditionals was that the code could be read independently of the lang. Have aliases not been considered then due to a lack of popularity of ::a/n syntax?

My suggestion would be a knob that reads unresolvable alias kws as a special object #aliased-keyword{:alias "a", :name "n"}. This would solve both Viktors problem and also pay its debt to the reader conditional intent.

Comment by Alex Miller [ 25/Jan/17 5:16 PM ]

1. The reader reads everything, it's just selective about which parts of an expression gets returned. Without reading, it wouldn't know where the end of that form was.

2. I think this is a little more subtle in that reading the second thing in a conditional requires remembering something read and discarded in a prior conditional.

That said, maybe it would be possible to either read in a mode where this particular case doesn't throw or where this particular exception was known and discarded when inside a conditional.





[CLJ-2102] Reduce collection generator default size from 20 Created: 25/Jan/17  Updated: 25/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: generator, spec

Attachments: Text File clj-2102.patch    
Patch: Code
Approval: Vetted

 Description   

In general I find that it is very easy (especially with nested or recursive collections) to have a check run OOME due to generating very large nested collections. Currently the default is 20 - I think we should change it to 3.

The attached patch just changes the default from 20 to 3. An alternate approach would be to change it to a dynvar setting.

Patch: clj-2102.patch






[CLJ-2101] Request for a way to forget specs Created: 24/Jan/17  Updated: 25/Jan/17  Resolved: 24/Jan/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Johan Gall Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: spec


 Description   

Hello,

I have a server that loads code, and specs, at runtime in generated namespaces.

Thus I have a spec leak.

It would be nice if there was an official API to forget specs.



 Comments   
Comment by Johan Gall [ 24/Jan/17 7:01 AM ]

(or better, a way to create a temporary spec world, but then that is probably not trivial)

Comment by Alex Miller [ 24/Jan/17 7:51 AM ]

Is this covered by http://dev.clojure.org/jira/browse/CLJ-2060 ?

Comment by Alex Miller [ 24/Jan/17 12:51 PM ]

Marking as a dup for now, but will re-open if this goes beyond what's in CLJ-2060.

Comment by Johan Gall [ 25/Jan/17 5:20 AM ]

ah sorry, indeed it's a dup





[CLJ-304] clojure.repl/source does not work with deftype Created: 20/Apr/10  Updated: 19/Jan/17

Status: In Progress
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 21
Labels: repl

Approval: Triaged

 Description   

clojure.repl/source does not work on a deftype

user> (deftype Foo [a b])
user.Foo
user> (source Foo)
Source not found

Cause: deftype creates a class but not a var so no file/line info is attached anywhere.

Approach:

Patch:

Screened by:



 Comments   
Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

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

Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

chouser@n01se.net said: That's a great question. get-source just needs a file name and line number.

If IMeta were a protocol, it could be extended to Class. That implementation could look for a "well-known" static field, perhaps? __clojure_meta or something? Then deftype would just have to populate that field, and get-source would be all set.

Does that plan have any merit? Is there a better place to store a file name and line number?

Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

stu said: Seems like a reasonable idea, but this is going to get back-burnered for now, unless there is a dire use case we have missed.

Comment by Gary Trakhman [ 19/Feb/14 10:31 AM ]

I could use this for cider's file/line jump-around mechanism as well.

With records, I can work around it by deriving and finding the corresponding constructor var, but it's a bit nasty.

Comment by Bozhidar Batsov [ 03/Mar/14 6:37 AM ]

I'd also love to see this fixed.

Comment by Andy Fingerhut [ 03/Mar/14 8:33 AM ]

Bozhidar, voting on a ticket (clicking the Vote link in the right of the page when viewing the ticket) can help push it upwards on listings of tickets by # of votes.

Comment by Bozhidar Batsov [ 19/Sep/14 1:17 PM ]

Andy, thanks for the pointer. They should have made this button much bigger, I hadn't noticed it all until now.

Comment by Alex Miller [ 26/Jan/16 4:12 PM ]

If someone did some work on this (like a patch), I would push it harder.

Comment by Baptiste Dupuch [ 19/Jan/17 2:45 PM ]

it's "working" for me using this =>

(source source-training.source/->Foo)




[CLJ-2100] s/nilable form should include the original spec, not the resolved spec Created: 19/Jan/17  Updated: 19/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File clj-2100.patch    
Patch: Code
Approval: Vetted

 Description   
=> (clojure.spec/def :test/name string?)
:test/name

=> (clojure.spec/form (clojure.spec/and :test/name))
(clojure.spec/and :test/name)

=> (clojure.spec/form (clojure.spec/nilable :test/name))
(clojure.spec/nilable clojure.core/string?)

In the final line, s/nilable form has the resolved spec rather than the original spec.

Proposed: Instead of getting the internal spec description, resolve the original spec form.

After:

user=> (clojure.spec/form (clojure.spec/nilable :test/name))
(clojure.spec/nilable :test/name)

Patch: CLJ-2100.patch






[CLJ-1771] Support for multiple key(s)-value pairs in assoc-in Created: 29/Jun/15  Updated: 14/Jan/17

Status: Reopened
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Griffin Smith Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: None
Environment:

All


Attachments: Text File clj-1771.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

It would be nice if assoc-in supported multiple key(s)-to-value pairs (and threw an error when there were an even number of arguments, just like assoc):

user=> (assoc-in {} [:a :b] 1 [:c :d] 2)
{:a {:b 1}, :c {:d 2}}
user=> (assoc-in {} [:a :b] 1 [:c :d])
IllegalArgumentException assoc-in expects even number of arguments after map/vector, found odd number

Patch: clj-1771.patch

Prescreened by: Alex Miller



 Comments   
Comment by Matthew Gilliard [ 23/Jul/15 2:15 PM ]

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

Comment by Yehonathan Sharvit [ 19/Aug/16 10:19 AM ]

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

Comment by Alex Miller [ 27/Nov/16 10:33 PM ]

Do you need the "if kvs" check?

Should have tests.

Comment by Matthew Gilliard [ 14/Jan/17 11:34 AM ]

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)





[CLJ-2098] autodoc fails to load clojure/spec.clj Created: 12/Jan/17  Updated: 13/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File clj-2098.patch    
Patch: Code
Approval: Vetted

 Description   

Reported by Tom F for the autodoc process. The following (essentially) is what autodoc is doing that currently blows up:

tom@renoir:~/src/clj/autodoc-work-area/clojure/src$ java -cp clojure.jar clojure.main
Clojure 1.9.0-master-SNAPSHOT
user=> (load-file "src/clj/clojure/spec.clj")
CompilerException java.lang.IllegalArgumentException: No implementation of method: :conform* \
of protocol: #'clojure.spec/Spec found for class: clojure.spec$regex_spec_impl$reify__14279, \
compiling:(/home/tom/src/clj/autodoc-work-area/clojure/src/src/clj/clojure/spec.clj:684:1)

Cause: There is code in Compiler.macroexpand() with the intention to suspend spec checking inside clojure.spec. However, it is currently doing an exact path match on SOURCE_PATH. In the load-file call above, this ends up being an absolute path.

Approach: Check for a path suffix rather than an exact match in Compiler.

Patch: clj-2098.patch






[CLJ-2089] Sorted colls with default comparator don't check that first element is Comparable Created: 19/Dec/16  Updated: 13/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File clj-2089.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Sorted maps and sets use the default comparator. The default comparator requires elements to be Comparable. PersistentTreeMap will not actually invoke the comparator until the second element is added to the map/set, so it is possible to create an invalid single element sorted map/set that will blow up only on later use.

The following examples create invalid "timebomb" collections that will throw ClassCastException if another element is added or if they're used in other ways (because sets are not comparable):

(sorted-set #{1})
(conj (sorted-set) #{1})
(sorted-map #{} 1)
(assoc (sorted-map) #{} 1)

Example:

(def s (sorted-set #{1}))  ;; this doesn't fail
(conj s #{2})              ;; first conj triggers error
ClassCastException clojure.lang.PersistentHashSet cannot be cast to java.lang.Comparable  clojure.lang.Util.compare (Util.java:153)

Cause: In PersistentTreeMap.add(), in the case where the existing tree is null, the comparator is never invoked and so the default comparator will never throw the ClassCastException seen on later compares. Note that none of this applies for a custom comparator (sorted-set-by and sorted-map-by) - those comparators can do whatever.

Proposed: In add(), if the map is empty AND the comparator is the default comparator, then check whether the added key is Comparable and throw CCE if not. Note that PersistentTreeMap is also the impl used by PersistentTreeSet so this covers both. The error message is customized to give you a better hint about the problem (and written to be applicable for both maps and sets). In the patch some existing (bad) tests had to be adjusted.

user=> (def s (sorted-set #{1}))
ClassCastException Default comparator requires nil, Number, or Comparable: #{1}  clojure.lang.PersistentTreeMap.add (PersistentTreeMap.java:335)

One downside of the current patch is that does not catch the equivalent problem if using a custom comparator and a bad first element. You could maybe do this by calling comp.compare(k,k) (although many comparators, like the default comparator, have an early identity check that would not fail on this). For now, decided that if you supply a custom comparator, it's up to you to not use it with elements that don't work with that comparator.

Patch: clj-2089.patch






[CLJ-1368] Document usage for case with non-readable constants Created: 02/Mar/14  Updated: 13/Jan/17

Status: Reopened
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docs, interop


 Description   

Problem

It is pretty obscure how to get constant-time dispatch for e.g. Enums, even if user knows about case.

Proposal

The possibility to dispatch to arbitrary constants with case, by wrapper macro, should be documented.

Wording

  • Should it warn against doing that with unstable values?
  • Should it mention anything else than java Enums?

Case Techniques

Case is documented for accepting all readable forms as test-constants. However, it can also be made to use any compile-time-known constants as test-constants, by wrapping it in another macro.

Sometimes this is appropriate, e.g. when dispatching on a java Enum.
Other times, less so, e.g. when dispatching on objects whose hash changes when the vm is restarted (breaks AOT).

Implications

This technique is an application of a more general technique: Passing non-literals to a macro from another macro.
Are there other macros that have use cases like this?

References

https://groups.google.com/d/topic/clojure/3yGjDO2YnjQ/discussion



 Comments   
Comment by Herwig Hochleitner [ 02/Mar/14 11:25 AM ]

This is a duplicate of http://dev.clojure.org/jira/browse/CLJ-1367

Actually, it's an alternate solution

Comment by Petr Gladkikh [ 13/Jan/17 5:58 AM ]

Probably this ticket and CLJ-1367 linger for so long because there's already 'condp' that can be used as follows:

(condp = test-value
  JavaClass/CONST1 result1
  JavaClass/CONST2 result2)

This is sequential and slower but is about as concise as plain case.

However if this is the form to be used instead of plain 'case' this should be suggested by documentation.





[CLJ-2097] Improve generation failure exception Created: 10/Jan/17  Updated: 11/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Russell Mull Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: generator, spec


 Description   

It's pretty easy to write a spec whose generator fails like this:

Couldn't satisfy such-that predicate after 100 tries.

This is of course expected in many ways, but it's a very unhelpful error. Some things that could make this better include:

  • Including the spec that failed in the exception. I only see one invocation of gen/such-that in spec.clj, and it appears to have the spec's form at hand. gen/such-that takes an exception constructor where this could be used.
  • Allow max-tries to be changed from the hardcoded value of 100. When dealing with an intermittent failure, it can be useful to crank down max-tries to a very small number, making the failure easier to reproduce.


 Comments   
Comment by Alex Miller [ 11/Jan/17 8:41 AM ]

These are reasonable suggestions and this area is likely to evolve in tandem with test.check to provide better info.





[CLJ-2074] ::keys spec conflicts with destructuring spec Created: 02/Dec/16  Updated: 11/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Alex Miller
Resolution: Unresolved Votes: 1
Labels: destructuring, spec

Attachments: File close-destructuring-keys-specs.diff    
Patch: Code

 Description   

As a consequence of the destructuring specs being implemented in terms of `s/keys`, defining a spec for `::keys` or `::strs` is problematic at the moment, because it will conflict with trying to use `::keys` for destructuring:

user=> (require '[clojure.spec :as s])
nil
user=> (s/def ::keys nil?)
:user/keys
user=> (let [{::keys [a]} {::a 1}] a)
ExceptionInfo Call to clojure.core/let did not conform to spec:
In: [0 0] val: #:user{:keys [a]} fails spec: :clojure.core.specs/local-name at: [:args :bindings :binding :sym] predicate: simple-symbol?
In: [0 0 0] val: ([:user/keys [a]]) fails spec: :clojure.core.specs/seq-binding-form at: [:args :bindings :binding :seq] predicate: (cat :elems (* :clojure.core.specs/binding-form) :rest (? (cat :amp #{(quote &)} :form :clojure.core.specs/binding-form)) :as (? (cat :as #{:as} :sym :clojure.core.specs/local-name))),  Extra input
In: [0 0 :user/keys] val: [a] fails spec: :user/keys at: [:args :bindings :binding :map :user/keys] predicate: nil?
:clojure.spec/args  ([#:user{:keys [a]} #:user{:a 1}] a)
  clojure.core/ex-info (core.clj:4725)

This feels like an implementation detail leak.



 Comments   
Comment by Brandon Bloom [ 10/Jan/17 5:36 PM ]

I also just ran in to this problem. Just wanted to say that I'd like to see a fix, but I'm not quite sure about the proposed solution. Or, at least, the name "closed?" seems to imply a non-extensible map, when in reality the flag more or less means "not a map that participates in the global keys system", for which I do not have a better name suggestion.

Comment by Alex Miller [ 11/Jan/17 8:35 AM ]

The proposed patch is a non-starter. I have some ideas on how to address this, but just haven't gotten around to working on it yet.

Comment by Alex Miller [ 11/Jan/17 8:37 AM ]

Removed proposal and patch from the ticket as we will not be going this direction. Captured here for reference:

"The attached patch implements a proposed solution to this issue, by adding a `:closed?` option to `s/keys` and using it for the destructuring spec. If `s/keys` is used with `:closed?` set to true, `conform` will only validate declared specs as opposed to the default behaviour of `s/keys` of validating all namespaced keywords with existing specs.

After this patch, the above example runs fine and usages of `s/keys` without `:closed?` set to true will validate against `::keys` as per current behaviour.

Patch: close-destructuring-keys-specs.diff"





[CLJ-2091] clojure.lang.APersistentVector#hashCode is not thread-safe Created: 24/Dec/16  Updated: 10/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: thurston n Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, concurrency

Attachments: File clj-2091-0.diff     File clj-2091-default-initialization.diff    
Patch: Code
Approval: Prescreened

 Description   

Problem: clojure.lang.APersistentVector#hashCode contain a deliberate data race on hash computation. However, the code as written does not follow safe practices for the intended data race. Specifically, the problem arises because the hashCode() (and hasheq()) method make multiple reads of the (unsynchronized) _hash field. The JMM permits these reads to return different values. Specifically, the last read in the return may return the pre-computed value -1, which is not the desired hash value. This problem also applies to APersistentMap, APersistentSet, and PersistentQueue.

See: http://jeremymanson.blogspot.com/2008/12/benign-data-races-in-java.html for a good description of the problem.

Fix: The main fix is to read the cached hash field only once and return the value of the local computation, not the value of the field.

A secondary change that is also beneficial is to use the default initializer value (which has special ordering in the JMM to the beginning of the thread) rather than setting and using -1 as the sentinel value.

In both cases these changes follow the canonical idioms used in java.lang.String for lazy hash computation. The patch covers both.

Patch: clj-2091-default-initialization.diff - note that this patch will indicate whitespace errors when applied due to the wacky line endings in PersistentQueue. The problem here is really the PQ formatting, not the patch.

Prescreened by: Alex Miller

There are some hash-related tests already but I also spot-checked that hash computations are returning the same value with and without the patch for the collections in question.



 Comments   
Comment by thurston n [ 24/Dec/16 4:38 PM ]

I can of course provide a patch, but as this is my first issue and am generally unfamiliar with clojure's practices and because this issue is not restricted to APersistentVector#hashCode, I thought it best to hold off and let the stewards decide how to proceed and how I could best help

Comment by Alex Miller [ 31/Dec/16 12:47 PM ]

Patch welcome (but please sign the contributor's agreement first - http://clojure.org/community/contributing). Also, see the processes for developing patches at http://dev.clojure.org/display/community/Developing+Patches.

AFAICT, the affected classes are APersistentVector, APersistentMap, APersistentSet, and PersistentQueue? Would be ok by me to cover all of them in a single patch.

Comment by thurston n [ 03/Jan/17 4:00 PM ]

AFAICT, the affected classes are APersistentVector, APersistentMap, APersistentSet, and PersistentQueue?

  • Dunno. However my experience tells me that the broken idiom (racy cache/memoization) is likely elsewhere; but I know of no systematic way of finding them. Regardless, I'll just focus on those 4 classes.
  • My plan is to also fix #hasheq(). It's the same problem; if you don't want that then just let me know and I'll refrain.
  • I'm not planning to deal with the initialization of #_hash and #_hasheq (currently inline-initialized to -1); that's a separate (although related) thread-safety problem. Might they be just what we refer to as a "legacy idiosyncrasy"? If so, then they really should be changed to just be default-initialized. I did, as an experiment, change one to default initialization, and the tests passed - that should be enough, but given that the persistent classes are serializable, code-coverage, et al., I can't say for sure. So I'll leave it to others more familiar with the codebase to make that determination. I note that if it is in future determined to change them, then #hashCode() and #hasheq() will need to be modified (trivially) accordingly.

That's the plan. Sound good?

Comment by Alex Miller [ 03/Jan/17 9:49 PM ]

I thought the non-default initialization was part of what you were describing, so now I'm not sure we're on the same page. Maybe you can just patch one so we have something concrete to talk about.

Comment by thurston n [ 03/Jan/17 9:56 PM ]

I'm not sure what you mean by "patch one" - I just submitted a patch, did you look at that?

Comment by Alex Miller [ 03/Jan/17 10:13 PM ]

I meant one class - sorry, I didn't see the patch. I will look at it tomorrow with fresh eyes.

Comment by thurston n [ 03/Jan/17 11:02 PM ]

Sure.

To be clear, as I mentioned in today's earlier comment, I would advise removing the inline-initialization, viz.

int _hash = -1;
int _hasheq = -1;

with

int _hash;
int _hasheq;

As I wrote, the extant tests would pass (of course, changing #hashCode() and #hasheq() appropriately)

But the initialization issue is a different, although certainly not orthogonal, issue than the one my patch addresses.

Currently, (i.e. pre-patch), #hashCode() can return a spurious -1 even if an APersistentVector instance is safely published - my patch fixes that.

However, because of the inline-initialization, an APersistentVector instance that is not safely published could return a spurious 0 from #hashCode(), even with my patch.

Now if the inline-initialization is just a "legacy idiosyncrasy" (and we all do that at one time or another), then it could be safely replaced (along with the appropriate modification to my patch) and all APersistentVector instances (safely published or not), would have #hashCode() implementations that are correct.

Comment by Alex Miller [ 06/Jan/17 3:19 PM ]

Ok, I went over all this again and it makes sense to me. I think you should proceed and also make the initializer change (remove the -1 as sentinel and replace with no initializer and 0 for the comparison checks in the methods).

Comment by thurston n [ 06/Jan/17 11:36 PM ]

Combines the 2 commits into a single commit patch
So incorporates the original patch changes (single read) with default initialization and checks for zero
Don't know what to do with PersistentQueue's mixed line-endings – that I'll leave to you to deal with

Comment by thurston n [ 09/Jan/17 8:16 PM ]

Problem also in core.lang.ASeq#hashCode() and core.lang.ASeq#hasheq() - although thankfully without inline initialization

Surely not the last place either

Comment by Alex Miller [ 10/Jan/17 8:33 AM ]

Feel free to update the patch if you like





[CLJ-1042] [PATCH] Allow negative substring indices for (subs) Created: 14/Aug/12  Updated: 09/Jan/17  Resolved: 17/Sep/12

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Ian Eure Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: enhancement, patch

Attachments: Text File clj-1042-negative-indices-patch3.txt     Text File negative-subs.patch    
Patch: Code and Test

 Description   

This adds Python-style negative string indices for (subs), e.g.:

(subs "foo bar" -3) ;; -> "bar"



 Comments   
Comment by Andy Fingerhut [ 16/Aug/12 7:17 PM ]

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

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

Comment by Ian Eure [ 20/Aug/12 11:44 AM ]

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

Comment by Andy Fingerhut [ 27/Aug/12 7:56 PM ]

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

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

Comment by Ian Eure [ 28/Aug/12 11:47 AM ]

Thanks, will do.

Comment by Steve Miner [ 04/Sep/12 3:53 PM ]

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

Comment by Ian Eure [ 06/Sep/12 12:17 PM ]

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

Comment by Adrian Bendel [ 07/Sep/12 8:01 AM ]

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

Comment by Steve Miner [ 07/Sep/12 1:31 PM ]

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

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

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

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

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

Comment by Adrian Bendel [ 07/Sep/12 11:05 PM ]

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

Your solution looks good.

Comment by Stuart Halloway [ 17/Sep/12 7:07 AM ]

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

Comment by Andy Fingerhut [ 17/Sep/12 12:07 PM ]

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

Comment by Ian Eure [ 18/Sep/12 12:31 PM ]

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

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

Comment by Andy Fingerhut [ 18/Sep/12 2:52 PM ]

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

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

Comment by Ian Eure [ 18/Sep/12 4:02 PM ]

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

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

Comment by Michael Klishin [ 18/Sep/12 5:09 PM ]

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

Comment by Carlos Cunha [ 18/Sep/12 5:10 PM ]

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

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

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

Comment by Steve Miner [ 18/Sep/12 5:25 PM ]

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

Comment by Stuart Halloway [ 19/Sep/12 12:03 PM ]

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

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

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

Comment by Andy Fingerhut [ 19/Sep/12 1:34 PM ]

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

Comment by Alex K [ 09/Jan/17 10:59 AM ]

This would have been a smart patch to make because four years later we wouldn't still be forced to write `(subs s 0 (dec (count s)))`.

Comment by Alex Miller [ 09/Jan/17 1:44 PM ]

Through the magic of "making your own function" technology, you only needed to write that once. And as a bonus, there was no need to consider all the consequences of adding it to core!





[CLJ-1865] Direct linking doesn't work on recursive calls Created: 08/Dec/15  Updated: 06/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, directlinking


 Description   

It looks like self-recursive calls aren't optimized by direct linking, but if we redefine the same function twice, the Compiler is tricked into thinking that the call is not recursive and (rightfully) optimizes it into an invokeStatic.

I haven't investigated the cause but I suspect (and I might be wrong) it has to do with :arglist metadata potentially having different values when the Var is undefined vs when it's already bound.

[~]> cat test.clj
(ns test)

(defn a [x]
  (a x))
[~]> clj
Clojure 1.8.0-master-SNAPSHOT
user=> (compile 'test)
test
user=> ^D
[~]> cd classes
[~/classes]> javap -c test\$a
Compiled from "test.clj"
public final class test$a extends clojure.lang.AFunction {
  public static final clojure.lang.Var const__0;

  public static {};
    Code:
       0: ldc           #11                 // String test
       2: ldc           #13                 // String a
       4: invokestatic  #19                 // Method clojure/lang/RT.var:(Ljava/lang/String;Ljava/lang/String;)Lclojure/lang/Var;
       7: checkcast     #21                 // class clojure/lang/Var
      10: putstatic     #23                 // Field const__0:Lclojure/lang/Var;
      13: return

  public test$a();
    Code:
       0: aload_0
       1: invokespecial #26                 // Method clojure/lang/AFunction."<init>":()V
       4: return

  public static java.lang.Object invokeStatic(java.lang.Object);
    Code:
       0: getstatic     #23                 // Field const__0:Lclojure/lang/Var;
       3: invokevirtual #32                 // Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
       6: checkcast     #34                 // class clojure/lang/IFn
       9: aload_0
      10: aconst_null
      11: astore_0
      12: invokeinterface #37,  2           // InterfaceMethod clojure/lang/IFn.invoke:(Ljava/lang/Object;)Ljava/lang/Object;
      17: areturn

  public java.lang.Object invoke(java.lang.Object);
    Code:
       0: aload_1
       1: aconst_null
       2: astore_1
       3: invokestatic  #41                 // Method invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
       6: areturn
}

Redefining the same function twice makes it work.

[~]> cat test.clj
(ns test)

(defn a [x]
  (a x))

(defn a [x]
  (a x))
[~]> clj
Clojure 1.8.0-master-SNAPSHOT
user=> (compile 'test)
test
user=> ^D
[~]> cd classes
[~/classes]> javap -c test\$a
Compiled from "test.clj"
public final class test$a extends clojure.lang.AFunction {
  public static final clojure.lang.Var const__0;

  public static {};
    Code:
       0: ldc           #11                 // String test
       2: ldc           #13                 // String a
       4: invokestatic  #19                 // Method clojure/lang/RT.var:(Ljava/lang/String;Ljava/lang/String;)Lclojure/lang/Var;
       7: checkcast     #21                 // class clojure/lang/Var
      10: putstatic     #23                 // Field const__0:Lclojure/lang/Var;
      13: return

  public test$a();
    Code:
       0: aload_0
       1: invokespecial #26                 // Method clojure/lang/AFunction."<init>":()V
       4: return

  public static java.lang.Object invokeStatic(java.lang.Object);
    Code:
       0: aload_0
       1: aconst_null
       2: astore_0
       3: invokestatic  #30                 // Method invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
       6: areturn

  public java.lang.Object invoke(java.lang.Object);
    Code:
       0: aload_1
       1: aconst_null
       2: astore_1
       3: invokestatic  #30                 // Method invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
       6: areturn
}


 Comments   
Comment by Nicola Mometto [ 06/Jan/17 10:12 AM ]

I just took a quick look at this, not an easy one to fix as handling recursive calls would likely require 2 passes over the whole defn AST, one to determine whether the defn is direct-linkable and the other one to build the AST using StaticInvokeExpr rather than InvokeExpr (using a stub Method using the analysis info rather than reflecting)

Comment by Alex Miller [ 06/Jan/17 2:03 PM ]

Yeah, Rich is aware of this and it's not done yet due to the issues you mentioned. It's hard!





[CLJ-272] load/ns/require/use overhaul Created: 18/Feb/10  Updated: 06/Jan/17  Resolved: 06/Jan/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Declined Votes: 3
Labels: None


 Description   

Creating this ticket to describe various things people have wanted to change about how ns works:

Minimal needs

  1. there should be a primitive level of loading (presumably load) that just loads without question.
  2. the api should be unified across the ns and direct forms. No more keywords or quoting! So (use foo) not (use 'foo). This makes use et al macros, so there should also be new fn versions (maybe use*).

Other possibilities to discuss.

  1. Feature addressing the :like and :clone ideas from http://onclojure.com/2010/02/17/managing-namespaces/. I think I would prefer a single new option :clone which allows :only and :exclude features as subspecifiers.
  2. Convenience fn to unmap all names in a namespace?


 Comments   
Comment by Assembla Importer [ 24/Aug/10 9:27 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 9:27 AM ]

stu said: Suggestions from Volkan Yazici:

Hi,

I saw your "load/ns/require/use overhaul" ticket[1] and would like to
ask for a few extra overhaulings. I have a project called retop, and
here is its file hiearachy:

tr/edu/bilkent/cs/retop.clj
tr/edu/bilkent/cs/retop/km.clj
tr/edu/bilkent/cs/retop/graph.clj
tr/edu/bilkent/cs/retop/main.clj
tr/edu/bilkent/cs/retop/util.clj

In retop.clj, I have below ns definition.

(ns tr.edu.bilkent.cs.retop
(:gen-class)
(:import
(com.sun.jna
Function
Pointer)
(com.sun.jna.ptr
IntByReference)
(tr.edu.bilkent.cs.patoh
HyperGraph
HyperGraphException
Parititoning
ParititoningParameters))
(:load
"retop/util"
"retop/km"
"retop/graph"
"retop/main"))

And in every .clj file in retop/ directory I have below in-ns in the
very first line.

(in-ns 'tr.edu.bilkent.cs.retop)

The problems with the ns decleration are:

1) Most of the :import's in retop.clj only belong to a single .clj file.
For instance,

(tr.edu.bilkent.cs.patoh
HyperGraph
HyperGraphException
Parititoning
ParititoningParameters)

imports are only used by graph.clj. Yep, I can add an (import ...)
line just after the (in-ns ...), but wouldn't it be better if I can
specify that in (in-ns ...) form?

2) See (:load ...) clause in (ns ...) form. There are lots of
unnecessary directory prefixes. I'd be prefer something ala Common
Lisp's defpackage:

(:load
"packages" ; packages.clj
("retop"
"util" ; retop/util.clj
"km" ; retop/km.clj
"graph" ; retop/graph.clj
("graph"
"foo" ; retop/graph/foo.clj
"bar) ; retop/graph/bar.clj
"main")) ; retop/main.clj

Also, being able to use wildcards would be awesome.

3) There are inconsistencies between macros and functions. For instance,
consider:

(ns foo.bar.baz (:use mov))
(in-ns 'foo.bar.baz)
(use 'mov)

I'd like to get rid of quotations in both cases.

I'm not sure if I'm using the right tools and doing the right approach
for such a project. But if you agree with the above overhauling
requirements, I'd like to see them appear in the same assembla ticket as
well.

Comment by Assembla Importer [ 24/Aug/10 9:27 AM ]

stuart.sierra said: My requests:

1. If writing macros that do not evaluate their arguments, provide function versions that do evaluate their arguments.

2. Do not support prefix lists for loading Clojure namespaces. It's hard to parse with external tools.

3. Do not conflate importing Java classes with loading Clojure namespaces. They are fundamentally different operations with different semantics.

I have implemented some ideas in a macro called "need" at http://github.com/stuartsierra/need

Comment by Stuart Sierra [ 12/Dec/10 4:08 PM ]

Further requests:

Permit tools to read the "ns" declaration and statically determine the dependencies of a namespace, without evaluating any code.

Comment by Paudi Moriarty [ 28/Feb/12 3:56 AM ]

Permit tools to read the "ns" declaration and statically determine the dependencies of a namespace, without evaluating any code.

This would be great for building OSGi bundles where Bnd is currently not much help.

Comment by Alex Miller [ 06/Jan/17 2:02 PM ]

Just closing as we're not going to work something like this off of a ticket. Not commenting on value of anything here or whether or not we'll do something like this in the future.





[CLJ-1556] Add instance check functions to defrecord/deftype Created: 09/Oct/14  Updated: 06/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: defrecord, deftype

Attachments: Text File 0001-CLJ-1556-Generate-type-functions-with-instance-check.patch    
Patch: Code

 Description   

It is often necessarty to test for instance? on deftypes/defrecords, this patch makes the two macros automatically generate a type? function implemented as (fn [x] (instance? type x)), to complement ->type and map->type
Example:

user=>(deftype x [])
user.x
user=>(x? (x.))
true


 Comments   
Comment by Jozef Wagner [ 09/Oct/14 9:11 AM ]

What about camel cased types? predicate SomeType? does not look like an idiomatic type predicate. I suggest to have this type predicate function and its name optional, through e.g. :predicate metadata on a type name. Moreover, it is far more useful to have such predicate on protocols, rather than types.

Comment by Nicola Mometto [ 09/Oct/14 9:17 AM ]

I don't think camel cased types should pose any issue. we use ->SomeType just as fine, I don't see why SomeType? should be problematic.

I disagree that it's more useful to have a predicate for protocols since protocols are already regular Vars and it's just a matter of (satisfies? theprotocol x), the value of the predicate on types/record is to minimize the necessity of having to import the actual class

Comment by Pierre-Yves Ritschard [ 06/Jan/17 9:47 AM ]

This would be super useful, thanks.





[CLJ-2096] "Key must be integer" thrown when vector lookup done with short or byte Created: 04/Jan/17  Updated: 06/Jan/17  Resolved: 05/Jan/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8, Release 1.9
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Aaron Cummings Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

clojure-1.9.0-alpha14 and clojure-1.8.0 tested.



 Description   

Looking up a value in a vector with a long or integer index works:
([:a :b :c] (long 1)) => :b
([:a :b :c] (int 1)) => :b

However, lookups with short or byte fail with "IllegalArgumentException Key must be integer"
([:a :b :c] (short 1))
([:a :b :c] (byte 1))

Root cause seems to be clojure.lang.Util.isInteger() which returns true only if the argument is an instance of Integer, Long, clojure.lang.BigInt, or BigInteger. I think it would be more correct for clojure.lang.Util.isInteger() to be consistent with clojure.core/integer? which additionally returns true for both Short and Byte.



 Comments   
Comment by Alex Miller [ 05/Jan/17 8:24 AM ]

I don't see any good reason to make changes in this area so declining.

Comment by Aaron Cummings [ 05/Jan/17 8:44 AM ]

Hi Alex - the case where we ran into this exception was in parsing a binary file where the record descriptor is a byte, and we used this byte to do a lookup in a vector (the vector holding keywords which describe the record type). The workaround is pretty simple though; just cast the byte to an int.

Curiously, a map lookup like ({0 :a, 1 :b, 2 :c} (byte 1)) does work.

I wondering though if the non-support of short and byte lookups in vectors is intentional, and if so, the reason for this choice (I don't see any obvious problems, so perhaps Rich knows something I don't here). If instead this is an oversight, and is deemed not broken enough to fix, then I can accept that.

Comment by Alex Miller [ 06/Jan/17 8:26 AM ]

I would say that given that the check and error message exists, it is intentional. Certainly there is a performance impact to checking for a broader set of types.





[CLJ-1936] instrumented fdef with fspec unnecessarily invokes fspec generator Created: 28/May/16  Updated: 05/Jan/17  Resolved: 03/Nov/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Allen Rohner Assignee: Unassigned
Resolution: Declined Votes: 2
Labels: spec, test

Approval: Triaged

 Description   

With test.check is on the classpath, an instrumented fdef with fspec will invoke the generator for the fspec when invoked:

(require '[clojure.spec :as s] '[clojure.spec.test :as st])

(defn foo [fnn] (fnn 42))
(s/fdef foo :args (s/cat :f (s/fspec :args (s/cat :i integer?)
                                     :ret integer?)))

(foo #(do (println %) (when (even? %) 42)))
42
42

(st/instrument `foo)

(foo #(do (println %) (when (even? %) 42)))
-1
0
-1
0
0
-1
0
-1
ExceptionInfo Call to #'user/foo did not conform to spec:
In: [0] val: nil fails at: [:args :f :ret] predicate: integer?
:clojure.spec/args  (#object[user$eval12$fn__13 0x515c6049 "user$eval12$fn__13@515c6049"])
:clojure.spec/failure  :instrument
:clojure.spec.test/caller  {:file "NO_SOURCE_FILE", :line 8, :var-scope user/eval12}
  clojure.core/ex-info (core.clj:4725)

Without test.check, this fails:

user=> (foo #(do (println %) (when (even? %) 42)))
FileNotFoundException Could not locate clojure/test/check/generators__init.class or clojure/test/check/generators.clj on classpath.  clojure.lang.RT.load (RT.java:458)


 Comments   
Comment by Zach Oakes [ 28/May/16 9:01 PM ]

I think it would make sense to add something like core.typed's ^:no-check for this. For example:

(s/fdef ^:no-check foo :args (s/cat :f (s/fspec :args (s/cat :i integer?) :ret integer?)))

As a stopgap measure, I made a boot task that has a copy of clojure.spec.test/run-all-tests and modifies it to ignore vars with that metadata. That means I have to add it to the metadata in defn rather than fdef but it still seems to work.

Comment by Sean Corfield [ 01/Jun/16 9:32 PM ]

Yes, there are definitely situations where I would want argument / return spec checking on calls during dev/test but absolutely need the function excluded from generative testing.

Comment by Sean Corfield [ 01/Jun/16 9:38 PM ]

If you don't have test.check on your classpath, the call to s/instrument succeeds but then attempting to call foo fails with:

boot.user=> (defn foo [fnn] (fnn 42))
#'boot.user/foo
boot.user=> (s/fdef foo :args (s/cat :f (s/fspec :args (s/cat :i integer?)
       #_=>                                      :ret integer?)))
boot.user/foo
boot.user=> 

boot.user=> (foo #(do (println %) (when (even? %) 42)))
42
42
boot.user=> (s/instrument 'foo)
#'boot.user/foo
boot.user=> (foo #(do (println %) (when (even? %) 42)))

java.io.FileNotFoundException: Could not locate clojure/test/check/generators__init.class or clojure/test/check/generators.clj on classpath.

That is certainly unexpected and not very friendly!

Comment by Alex Miller [ 28/Jun/16 10:05 PM ]

There are new options in instrument as of 1.9.0-alpha8 that allow you to stub/mock functions. Those are one potential answer to this and maybe the recommended one, although I haven't used them enough to say that for sure.

Comment by Alex Miller [ 05/Jul/16 10:52 AM ]

See also CLJ-1976.

Comment by Sean Corfield [ 18/Aug/16 4:08 PM ]

Given that recent Alpha builds no longer check :ret or :fn with instrumentation, this issue seems to be resolved Alex Miller?

Comment by Allen Rohner [ 18/Aug/16 4:22 PM ]

fspec still requires generative testing.

Comment by Sean Corfield [ 18/Aug/16 4:24 PM ]

Ah, OK, I thought that had also rolled back to just :args testing at this point (I hadn't retested this since we have test.check as dev/test dependency now anyway).

Comment by Alex Miller [ 03/Nov/16 12:24 PM ]

If an argument takes a function and you pass it a function, instrument cannot validate the fspec other than by generating args for the fspec and trying it out. So, this is the intended behavior. So, this is working as intended.

Comment by Allen Rohner [ 03/Nov/16 12:59 PM ]

Instrument could wrap the function in an fn that checks arguments the same way instrument does. fspec being tied to generators makes it significantly less useful for functions with side effects.

Comment by Alex Miller [ 03/Nov/16 3:54 PM ]

That's an interesting idea, not sure if it satisfies though. Also note that instrument provides a number of options for specifying simpler instrumented replacement specs/stubs.

Comment by Brandon Bloom [ 05/Jan/17 3:33 PM ]

Following up from a discussion occurring now in Slack #clojure-spec. Lots more discussion there, check timestamp.

I found this very surprising. I've been using instrument during dev/repl-time (as recommended? distinguishing form test-time), but just learned that fspec isn't suitable for functions with side-effects due to the use of generators. I expected instrument to only instrument calls to vars, never to use the generators, and either 1) ignore fspec's details (easier) or 2) proxy the fn (many challenges here).

instrument has been very useful for dev (again, vs test), but not as useful as it could be if :ret and :fn were checked, and generators were never used.

Comment by Brandon Bloom [ 05/Jan/17 3:38 PM ]

Whoops submitted a moment too soon. I wanted to add: During dev, I'd rather more things be checked a little bit (ie ret and fn) than fewer things be tested thoroughly (ie higher order functions subjected to generated inputs).

I think a distinction needs to be drawn between what can be conformed with 100% confidence and what cannot. For pure data w/o functions, we can expect conform to make some promises. With fspec etc, we can't rely on valid? => true as a security measure. I'm fine with that, but different contexts/times require different levels of confidence.





[CLJ-2090] Improve clojure.core/distinct perf by using transient set Created: 23/Dec/16  Updated: 04/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance, transducers, transient

Attachments: Text File clj-2090-use-transient-set-in-distinct-2.patch    
Patch: Code
Approval: Triaged

 Description   

Current implementation of clojure.core/distinct uses persistent set. This patch improves performance of lazy arity by ~25%-30% and transducer by ~40%-50% by using transient set instead.

10 elements
(doall (distinct coll)) 	 5.773439 µs => 4.179092 µs (-27%)
(into [] (distinct) coll) 	 3.238236 µs => 1.943254 µs (-39%)

100 elements
(doall (distinct coll)) 	 67.725764 µs => 42.129993 µs (-37%)
(into [] (distinct) coll) 	 35.702741 µs => 16.495947 µs (-53%)

1000 elements
(doall (distinct coll)) 	 540.652739 µs => 399.053873 µs (-26%)
(into [] (distinct) coll) 	 301.423077 µs => 164.025500 µs (-45%)

10000 elements
(doall (distinct coll)) 	 3.439137 ms => 3.058872 ms (-11%)
(into [] (distinct) coll) 	 1.437390 ms => 848.277178 µs (-40%)

Benchmarking code: https://gist.github.com/tonsky/97dfe1f9c48eccafc983a49c7042fb21



 Comments   
Comment by Alex Miller [ 23/Dec/16 8:52 AM ]

You can't remove the volatile - you still need that for safe publication in multi threaded transducing contexts.

Comment by Nikita Prokopov [ 23/Dec/16 11:50 AM ]

Alex Miller How do you mean?

  • I don’t update seen link because transient set can be mutated in-place
  • Are transducers meant to be used from multiple threads? Because even existing implementation clearly has race condition. I imagine fixing that would be costly (we’ll need a synchronized section), so maybe it should be a specialized transducer that you use only when needed?
Comment by Alex Miller [ 23/Dec/16 12:26 PM ]

Transient sets can NOT be mutated in place - you must use the return value.

Yes, transducers are used from multiple threads in (for example) transducer chans in core.async go blocks.

Comment by Alex Miller [ 23/Dec/16 12:28 PM ]

I should also say transducers are not expected to be used from more than one thread at a time, so there are no race problems. But being used from multiple threads over time requires proper safe publication.

Comment by Nikita Prokopov [ 24/Dec/16 3:07 AM ]

But being used from multiple threads over time requires proper safe publication.

Does that imply that no transients could be used in transducers (because underlying arrays on which transient impl is based are mutated in place, so different threads could potentially see different states of transient object)?

Does that also mean that partition-by and partition-all should be fixed (they use java.util.ArrayList which, being array of references, has no safe publication semantics)?

Transient sets can NOT be mutated in place - you must use the return value.

I was thinking that clojure/core.clj and clojure.lang.ATransientSet.java are both part of Clojure internals, colocated, so can share a little bit of internal knowledge about each other. It seems safe to do that, because that knowledge does not leak outside, and, if at any point impl of ATransientSet would change, core.clj could be updated accordingly in the same release. I wouldn’t do that in any third-party library, of course.

Comment by Alex Miller [ 24/Dec/16 9:13 AM ]

Does that imply that no transients could be used in transducers (because underlying arrays on which transient impl is based are mutated in place, so different threads could potentially see different states of transient object)?

Transients require only that they are asked by no more than a single thread at a time and so are safe to use in a transducer. However, they should guarantee safe publication. core.async channels already do this as an artifact of their implementation, but other transducing contexts may not.

Transients should NEVER be used as "mutate in place", regardless of concurrency. While they will appear to "work" in some circumstances, this is never correct (eventually an update operation will return a new instance and if you are mutating in place, your data will then be missing). This is discussed and correct examples are shown at http://clojure.org/reference/transients.

Does that also mean that partition-by and partition-all should be fixed (they use java.util.ArrayList which, being array of references, has no safe publication semantics)?

That's something Rich and I are discussing but, probably.

Comment by Nikita Prokopov [ 24/Dec/16 12:56 PM ]

Alex Miller Here’s quick test that shows that changes to transient set (which is nothing more but a wrapper around transient map) made in one thread are not always visible from another thread.

https://gist.github.com/tonsky/62a7ec6d539fc013186bee2df0812cf6

That means that if we try to use transients for e.g. distinct it will miss duplicate items

Comment by Nikita Prokopov [ 24/Dec/16 1:02 PM ]

Removed transients from transducer arity of distincts because transducers might be accessed from multiple threads

Comment by Nikita Prokopov [ 24/Dec/16 1:12 PM ]

Maybe that doc http://clojure.org/reference/transients should be updated re: transients are not safe to use from multiple threads because changes made by one thread are not necessarily visible to another. Even if they don’t compete

Comment by Alex Miller [ 31/Dec/16 12:54 PM ]

I would say that test is demonstrating a bug in transient sets/maps and you should file a ticket for that as it's a lot more important than this enhancement.

distinct should be able to use transients in both the transducer and lazy seq impls. The issue with contains? not working on transients is actually a separate ticket - http://dev.clojure.org/jira/browse/CLJ-700 that will likely require some class hierarchy rearrangement. I don't think we would take this change until that is fixed (so that you can avoid relying on the class and Java method variants).

Comment by Nikita Prokopov [ 04/Jan/17 11:47 AM ]

I have to admit my test was demonstrating something else: there were no proper thread isolation. So it was a concurrency issue, not “safe publication” issue. My current understanding is this:

Transients require thread isolation. Use of a particular transient instance should be controlled either by using it in an single-threaded scope, or in a framework that enforces this.

That guarantee implicitly presumes that there’s happens-before relation between transient usage from multiple threads. There’s no other way to define “only one thread is in this section at a time”.

That, in turn, means that all writes that happened in thread 1 are visible in thread 2, regardless to volatility of the variables involved. In fact, we can remove all volatiles from transients implementation and probably make them faster, because, by asking “no more than one thread at a time” we enforce users to establish happens-before between sections, and that would give us all the safe publication guarantees we need.

Is my understanding correct? Am I missing something?

Comment by Nikita Prokopov [ 04/Jan/17 11:55 AM ]

Also, long-living transients (e.g. in a transducers associated with a queue, for example) will hold a reference to a thread that created them. Is that a bad thing? Should we switch to boolean flag instead?





[CLJ-2095] Doc s/gen overrides do not take effect inside custom generators Created: 03/Jan/17  Updated: 03/Jan/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Russell Mull Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, generator, spec
Environment:

clojure 1.9.0-alpha14



 Description   

Custom generators may build (via fmap/bind) on spec generators. Generator overrides at the top level will not take effect inside custom generators:

(require '[clojure.spec :as s])
(require '[clojure.test.check.generators :as gen])

;; A map that holds a single integer value
(s/def ::val integer?)
(s/def ::body (s/keys :req [::val]))

;; This spec matches stringified versions of 'body'.
;; (read-string is for demonstration purposes only, of course)
(s/def ::stringy-body
  (s/with-gen
    (s/and string? #(s/valid? ::body (read-string %)))
    #(gen/fmap pr-str (s/gen ::body))))

(s/valid? ::stringy-body "{:user/val 37}") ;; => true

;; This makes various stringified maps, as expected
(take 3 (gen/sample (s/gen ::stringy-body)))
;; => ("#:user{:val -1}" "#:user{:val 0}" "#:user{:val -1}")

;; *** But the overrides don't get passed through ***
(take 3 (gen/sample (s/gen ::stringy-body {::val #(s/gen #{42})})))
;; ("#:user{:val -1}" "#:user{:val 0}" "#:user{:val 0}")

Should consider documenting this in s/gen, s/with-gen, etc.



 Comments   
Comment by Alex Miller [ 03/Jan/17 5:39 PM ]

When you use with-gen, you're basically overriding the built-in gen mechanism (which supports overrides) and providing your own (opaque to spec) generator. You should not expect overrides to take effect inside a custom generator.

Comment by Russell Mull [ 03/Jan/17 5:41 PM ]

That makes sense, but in lieu of that I expected (and went looking for) some way to get at the overrides map from the function passed to s/with-gen, and found none.

Comment by Russell Mull [ 03/Jan/17 5:48 PM ]

... I didn't fully parse your comment the first time around. I can see from the implementation that a custom generator (gfn internally) is never passed any of the contextual information that the builtin specs have at hand. As it sounds like this is intentional, it would be useful to note this limitation in the docstring for s/gen or perhaps s/with-gen.

Comment by Alex Miller [ 03/Jan/17 5:58 PM ]

It's not a crazy idea, but it doesn't seem like there's any way this could be done in the current impl without some pretty significant changes.





[CLJ-2094] clojure.spec: bug with protocols and extend-type Created: 01/Jan/17  Updated: 03/Jan/17  Resolved: 03/Jan/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Defect Priority: Major
Reporter: John Schmidt Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: spec


 Description   

I have the following two clj files (I've tried to come up with a minimal example):

core.clj
--------------
(ns spec-test.core
  (:require [clojure.spec :as s]))

(defprotocol Game
  (move [game]))

(s/def ::game1 #(satisfies? Game %))
(s/def ::game2 (partial satisfies? Game))

foo.clj
--------------
(ns spec-test.foo
  (:require [spec-test.core :refer [Game]]))

(defrecord Foo [])

(extend-type Foo
  Game
  (move [game]))

Here's a REPL session that explains my problem:

user=> (ns spec-test.core)
nil
spec-test.core=> (require 'spec-test.core :reload)
nil
spec-test.core=> (require 'spec-test.foo :reload)
nil
spec-test.core=> (satisfies? Game (spec-test.foo/->Foo))
true
spec-test.core=> ((partial satisfies? Game) (spec-test.foo/->Foo))
true
spec-test.core=> (s/explain ::game1 (spec-test.foo/->Foo))
Success!
nil
spec-test.core=> (s/explain ::game2 (spec-test.foo/->Foo))
val: #spec_test.foo.Foo{} fails spec: :spec-test.core/game2 predicate: (partial satisfies? Game) <---- WAAAAAT
nil

I would expect ::game1 and ::game2 to be equivalent, but somehow they're not.

Also see: https://groups.google.com/forum/#!topic/clojure/igBlMpqGU3A



 Comments   
Comment by Steve Miner [ 02/Jan/17 12:43 PM ]

I gave some incorrect advice on the mailing list so I'll try to correct myself here. Basically, the protocol is stored in a var, Game. If a predicate captures a particular state of the protocol, it won't respond correctly when additions are made to the protocol (with extend-type, etc.) So the problem with the usage of `partial` is that it evaluates the current value of the protocol at that point in time, before it has been extended to cover Foo.

The #(...) function definition would have used the var itself, not its current value. Naturally, the var allows the protocol to be updated such that the function sees the updated value. Basically, this is just normal Clojure evaluation. A `defn` style function would have worked fine as well. It's just the `partial` that evaluated its args that leads to the problem.

The same kind of issue could come up if you passed any var to partial and captured the current value of the var. Later changes to the var would not affect the result of partial.

I'll say the bug is the confusing error message. It seems that s/explain is implying it is using the var Game, but it really captured an "old" value of Game.

Comment by Alex Miller [ 03/Jan/17 2:10 PM ]

I think Steve's assessment is all correct.

I'm not sure that it's possible for spec to know what's happened here though wrt giving a better error message.

I don't think I really see anything that can be "fixed", so I'm going to mark this as declined.





[CLJ-2086] A macro in a nested syntax-quote causes an exception Created: 15/Dec/16  Updated: 03/Jan/17  Resolved: 15/Dec/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: N/A Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

(defmacro foo
[x y]
``(~~x ~~y))

(foo and true)

CompilerException java.lang.RuntimeException: Can't take value of a macro: #'clojure.core/and



 Comments   
Comment by Alex Miller [ 15/Dec/16 10:15 PM ]
user=> (macroexpand '(foo and true))
(clojure.core/seq (clojure.core/concat (clojure.core/list and) (clojure.core/list true)))

What do you expect this to do?

Comment by Alex Miller [ 15/Dec/16 10:36 PM ]

Declining till more info about a) what problem you are trying to solve and b) what the expected behavior is.

Comment by N/A [ 22/Dec/16 8:30 PM ]

a) I'm trying to define a macro that defines a macro.
(defmacro make-defmacro
[macro-name macro]
`(defmacro ~macro-name
x# & more#
(do (~macro x#)
`(~~macro-name ~@more#))))

(make-defmacro bar and)
CompilerException java.lang.RuntimeException: Can't take value of a macro

b) (defmacro foo
[x y]
``(~~x ~~y))

(macroexpand '(foo and true))
=> (and true)

Comment by Viktor Magyari [ 03/Jan/17 10:52 AM ]

Try

(defmacro foo [x y]
  ``(~'~x ~~y))




[CLJ-2092] deftype instances with mutable fields cannot be compiled Created: 24/Dec/16  Updated: 31/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, deftype

Approval: Triaged

 Description   

When evaluating or compiling an implementer of clojure.lang.IType, the compiler tries to reflectively access its fields. This fails, when a field is marked mutable (hence private):

Clojure 1.9.0-master-SNAPSHOT
user=> (deftype T [^:unsynchronized-mutable t])
user.T
user=> (T. :t)
#object[user.T 0x2654635 "user.T@2654635"]
user=> (eval (T. :t))
CompilerException java.lang.IllegalArgumentException: No matching field found: t for class user.T
            Reflector.java:  271  clojure.lang.Reflector/getInstanceField
             Compiler.java: 4724  clojure.lang.Compiler$ObjExpr/emitValue
             Compiler.java: 4851  clojure.lang.Compiler$ObjExpr/emitConstants
             Compiler.java: 4529  clojure.lang.Compiler$ObjExpr/compile
             Compiler.java: 4049  clojure.lang.Compiler$FnExpr/parse
             Compiler.java: 6866  clojure.lang.Compiler/analyzeSeq
             Compiler.java: 6669  clojure.lang.Compiler/analyze
             Compiler.java: 6924  clojure.lang.Compiler/eval
             Compiler.java: 6890  clojure.lang.Compiler/eval
                  core.clj: 3105  clojure.core/eval
...

For classes that don't implement IType, no such problem exists.

user> (deftype* user/U user.U
        [^:unsynchronized-mutable u]
        :implements [])
nil
user> (eval (user.U. :u))
#object[user.U 0x34699051 "user.U@34699051"]

This problem commonly occurs, when implementing a tagged literal for a deftype with cached hash.



 Comments   
Comment by Alex Miller [ 31/Dec/16 12:01 PM ]

Yeah, this is interesting. The compiler compiles a deftype into a call to the constructor with the current values of the fields, but mutable fields are not accessible. One alternative would be to provide some standard method to "read" the field set rather than relying on reflection. (Another would be changing the access modifiers for mutable fields but I think that's probably a non-starter.)





[CLJ-2093] partial and fn behave differently with eval Created: 28/Dec/16  Updated: 31/Dec/16  Resolved: 31/Dec/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: N/A Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

(eval (list map (partial + 1) [0]))
CompilerException java.lang.ExceptionInInitializerError
(eval (list map (fn [x] (+ 1 x)) [0]))
=> (1)



 Comments   
Comment by Kevin Downey [ 29/Dec/16 9:22 PM ]

same issue as http://dev.clojure.org/jira/browse/CLJ-1206 and all the issues connected to that

Comment by Alex Miller [ 31/Dec/16 11:17 AM ]

You should not expect either of these to work as the expressions contain function objects (not function forms).

You should be doing something like this:

(eval (list 'map '(partial + 1) [0]))




[CLJ-1575] Using a (def ^:const instance) of a deftype that implements IPersistentCollection, triggers compiler errors Created: 29/Oct/14  Updated: 24/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6, Release 1.7
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

fresh repl


Attachments: Text File 0001-Test-for-analyzer-bug-CLJ-1575.patch    

 Description   

The compiler has a lot of assumptions about the possible types of IPersistentCollection literals and rightfully so. The strange thing with this case is, that taking the (constant) value works as soon as count is defined, but using it as an argument hits a closed dispatch for emitting the empty variants of the various literals.

> (deftype T [] clojure.lang.IPersistentCollection (count [_] 0)
> (def ^:const t (T.))
> (meta t)
java.lang.UnsupportedOperationException: Unknown Collection type
Compiler.java:2860 clojure.lang.Compiler$EmptyExpr.emit
Compiler.java:3632 clojure.lang.Compiler$InvokeExpr.emitArgsAndCall
...

EDIT updated the ticket after some investigation
NOTE attached test patch doesn't even implement (count []) for the deftype, which just triggers a rightful AbstractMethodError



 Comments   
Comment by Herwig Hochleitner [ 29/Oct/14 10:00 PM ]

The test had a typo, sorry

Comment by Alex Miller [ 30/Oct/14 7:14 AM ]

Looks like a variant of CLJ-1093.

Comment by Herwig Hochleitner [ 24/Dec/16 8:27 PM ]

This bug is still present in 1.8, even though CLJ-1093 has been marked fixed for 1.8.





[CLJ-2075] Add three-arities to < <= > >= = == not= Created: 03/Dec/16  Updated: 23/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: None

Attachments: Text File clj-2075-add-three-arities-to-comparisons.patch     Text File clj-2075-over-clj-1912.patch    

 Description   

In my practice, using three-arities of less/greater operations is pretty common for e.g. checking a number is in range:

(< 0 temp 100)

The problem is, it is almost three times as slow compared to (and (< 0 temp) (< temp 100)).

This happens because three-arities are handled by the generic vararg arity branch:

(defn <
  "Returns non-nil if nums are in monotonically increasing order,
  otherwise false."
  {:inline (fn [x y] `(. clojure.lang.Numbers (lt ~x ~y)))
   :inline-arities #{2}
   :added "1.0"}
  ([x] true)
  ([x y] (. clojure.lang.Numbers (lt x y)))
  ([x y & more]
    (if (< x y)
     (if (next more)
       (recur y (first more) (next more))
       (< y (first more)))
     false)

This patch adds special handling for three-arities to these fns: < <= > >= = == not=

(defn <
  "Returns non-nil if nums are in monotonically increasing order,
  otherwise false."
  {:inline (fn [x y] `(. clojure.lang.Numbers (lt ~x ~y)))
   :inline-arities #{2}
   :added "1.0"}
  ([x] true)
  ([x y] (. clojure.lang.Numbers (lt x y)))
  ([x y z] (and (. clojure.lang.Numbers (lt x y))
                (. clojure.lang.Numbers (lt y z))))
  ([x y z & more]
   (if (< x y)
     (let [nmore (next more)]
       (if nmore
         (recur y z (first more) nmore)
         (< y z (first more))))
     false)))

The performance gains are quite significant:

(= 5 5 5) 	 24.508635 ns => 4.802783 ns (-80%)
(not= 1 2 3) 	 122.085793 ns => 21.828776 ns (-82%)
(< 1 2 3) 	 30.842993 ns => 6.714757 ns (-78%)
(<= 1 2 2) 	 30.712399 ns => 6.011326 ns (-80%)
(> 3 2 1) 	 22.577751 ns => 6.893885 ns (-69%)
(>= 3 2 2) 	 21.593219 ns => 6.233540 ns (-71%)
(== 5 5 5) 	 19.700540 ns => 6.066265 ns (-69%)

Higher arities also become faster, mainly because there's one less iteration now:

(= 5 5 5 5) 	 50.264580 ns => 31.361655 ns (-37%)
(< 1 2 3 4) 	 68.059758 ns => 43.684409 ns (-35%)
(<= 1 2 2 4) 	 65.653826 ns => 45.194730 ns (-31%)
(> 3 2 1 0) 	 119.239733 ns => 44.305519 ns (-62%)
(>= 3 2 2 0) 	 65.738453 ns => 44.037442 ns (-33%)
(== 5 5 5 5) 	 50.773521 ns => 33.725097 ns (-33%)

This patch also changes vararg artity of not= to use next/recur instead of apply:

(defn not=
  "Same as (not (= obj1 obj2))"
  {:tag Boolean
   :added "1.0"
   :static true}
  ([x] false)
  ([x y] (not (= x y)))
  ([x y z] (not (= x y z)))
  ([x y z & more]
   (if (= x y)
     (let [nmore (next more)]
       (if nmore
         (recur y z (first more) nmore)
         (not= y z (first more))))
     true)))

Results are good:

(not= 1 2 3 4) 	 130.517439 ns => 29.675640 ns (-77%)

I'm also doing what Jozef Wagner did in CLJ-1912 (calculating (next more) just once), although perf gains from that alone are not that big.

My point here is that optimizing three-arities makes sence because they appear in the real code quite often. Higher arities (4 and more) are much less widespread.



 Comments   
Comment by Nikita Prokopov [ 03/Dec/16 2:32 AM ]

Benchmark code here https://gist.github.com/tonsky/442eda3ba6aa4a71fd67883bb3f61d99

Comment by Alex Miller [ 03/Dec/16 8:24 AM ]

It might make more sense to combine this with CLJ-1912, otherwise these patches will fight.

Comment by Nikita Prokopov [ 03/Dec/16 1:02 PM ]

Use this patch if CLJ-1912 would be applied first

Comment by Nikita Prokopov [ 23/Dec/16 7:50 AM ]

I found a problem with previous patches that during defining = (equality), and is not yet defined. Replaced with if





[CLJ-1209] clojure.test does not print ex-info in error reports Created: 11/May/13  Updated: 22/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Critical
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: clojure.test

Attachments: Text File 0001-use-new-printing-method.patch     Text File 0002-CLJ-1209-show-ex-data-in-clojure-test.patch     File clj-test-print-ex-data.diff     Text File output-with-0002-patch.txt    
Patch: Code
Approval: Triaged

 Description   

clojure.test does not print the data attached to ExceptionInfo in error reports.

(use 'clojure.test)
(deftest ex-test (throw (ex-info "err" {:some :data})))
(ex-test)

ERROR in (ex-test) (core.clj:4591)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ExceptionInfo: err
 at clojure.core$ex_info.invoke (core.clj:4591)
    user/fn (NO_SOURCE_FILE:2)
    clojure.test$test_var$fn__7666.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)
    ...

Approach: In clojure.stacktrace, which clojure.test uses for printing exceptions, add a check for ex-data and pr it.

After:

ERROR in (ex-test) (core.clj:4591)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ExceptionInfo: err
{:some :data}
 at clojure.core$ex_info.invoke (core.clj:4591)
    user/fn (NO_SOURCE_FILE:3)
    clojure.test$test_var$fn__7667.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)

Patch: 0002-CLJ-1209-show-ex-data-in-clojure-test.patch



 Comments   
Comment by Alex Miller [ 20/Dec/13 9:53 AM ]

Great idea, thx for the patch!

Comment by Alex Miller [ 20/Dec/13 9:54 AM ]

Would be great to see a before and after example of the output.

Comment by Ivan Kozik [ 12/Jul/14 10:35 PM ]

Attaching sample output

Comment by Stuart Sierra [ 05/Sep/14 3:24 PM ]

As pointed out on IRC, there's a possible risk of trying to print an infinite lazy sequence that happened to be included in ex-data.

To mitigate, consider binding *print-length* and *print-level* to small numbers around the call to pr.

Comment by Stephen C. Gilardi [ 13/May/15 2:39 PM ]

http://dev.clojure.org/jira/browse/CLJ-1716 may cover this well enough that this issue can be closed.

Comment by Alex Miller [ 14/May/15 8:35 AM ]

I don't think 1716 covers it at all as clojure.test/clojure.stacktrace don't use the new throwable printing. But they could! And that might be a better solution than the patch here.

For example, the existing patch does not consider what to do about nested exceptions, some of which might have ex-data. The new printer handles all that in a consistent way.

Comment by Ed Bowler [ 22/Dec/16 11:35 AM ]

I think http://dev.clojure.org/jira/secure/attachment/16361/0001-use-new-printing-method.patch fixes the printing of the Exceptions.





[CLJ-1738] Document that seqs are incompatible with Java iterators that return the same mutable object every time Created: 27/May/15  Updated: 21/Dec/16  Resolved: 17/Jun/15

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: regression
Environment:

1.7.0-RC1


Attachments: Text File clj-1738-2.patch     Text File clj-1738-3.patch     Text File clj-1738-4.patch     Text File clj-1738-doc.patch     Text File clj-1738.patch    
Patch: Code
Approval: Ok

 Description   

Some Java libraries return iterators that return the same mutable object on every call:

  • Hadoop ReduceContextImpl$ValueIterator
  • Mahout DenseVector$AllIterator/NonDefaultIterator
  • LensKit FastIterators

While careful usage of seq or iterator-seq over these iterators worked in the past, that is no longer true as of the changes in CLJ-1669 - iterator-seq now produces a chunked sequence. Because next() is called 32 times on the iterator before the first value can be retrieved from the seq, and the same mutable object is returned every time, code on iterators like this now receives different (incorrect) results.

Approach: Sequences cache values and are thus incompatible with holding mutable and mutating Java objects. We will add some clarification about this to seq and iterator-seq docstrings. For those iterators above, it is recommended to either process those iterators in a loop/recur or to wrap them in a lazy-seq that transforms each re-returned mutable object into a proper value prior to caching.

Patch: clj-1738-doc.patch



 Comments   
Comment by Alex Miller [ 27/May/15 7:00 AM ]

I spot-checked some of the perf timings from CLJ-1669 and didn't see anything unexpected.

Comment by Marshall T. Vandegrift [ 27/May/15 7:38 AM ]

In order to maintain compatibility it is also necessary to change `clojure.lang.RT/seqFrom` back to creating non-chunked `IteratorSeq`s. I've verified that these changes are sufficient to restore compatibility for my cases.

Comment by Marshall T. Vandegrift [ 27/May/15 10:05 AM ]

Added updated version of proposed patch which covers RT Iterable->seq coercion and includes a test case.

Comment by Alex Miller [ 01/Jun/15 6:39 AM ]

The seqFrom change is good but I'd prefer not to add the Java class in the test. Can you replace that with a deftype implementing Iterable to reify an Iterator?

Comment by Marshall T. Vandegrift [ 01/Jun/15 10:32 AM ]

Added updated version of patch with pure-Clojure implementation of mutation-based iterator test.

Comment by Alex Miller [ 05/Jun/15 9:12 AM ]

I re-ran the full perf tests from CLJ-1669 and did not see any real changes except for in the last sort over eduction ones. We should still be seeing chunked iterator sequences over eductions which was the primary intent of the original change. We've just fallen back to non-chunked as we had before in the general case.

Comment by Alex Miller [ 05/Jun/15 2:39 PM ]

I think this looks good but since I had a hand in the early development of the patch I'm going to suggest that Stu screen it.

Comment by Alex Miller [ 09/Jun/15 1:18 PM ]

Marshall, can you update the patch so eduction's docstring says "reducible/iterable/seqable" and "reduce/iterator/seq"?

Comment by Marshall T. Vandegrift [ 09/Jun/15 3:08 PM ]

No problem. Updated and attached, but I've also changed the patch author to myself and fleshed out the commit message – if I'm going to do the drudge work I might as well take the credit too!

Comment by Alex Miller [ 09/Jun/15 3:16 PM ]

No problem at all - thanks!

Comment by Alex Miller [ 17/Jun/15 9:33 AM ]

Direction of this ticket changed at Rich's request.

Prior description capture here:

Clojure code that uses iterator-seq to wrap Java iterators that return the same mutable object on every call are broken by the chunked iterator-seq changes from CLJ-1669.

Some examples where this occurs:

  • Hadoop ReduceContextImpl$ValueIterator
  • Mahout DenseVector$AllIterator/NonDefaultIterator
  • LensKit FastIterators

Cause: In 1.6, the iterator-seq wrapper could be used with these to consume a sequence over these iterators element-by-element. In 1.7 RC1, iterator-seq produces a chunked sequence. Because next() is called 32 times on the iterator before the first value can be retrieved from the seq, and the same mutable object is returned every time, code doing this now receives different (incorrect) results.

Approach: Switch iterator-seq back to non-chunked and change eduction to use the chunking iterator-seq strategy as that was the original target. Retain the use of the chunked iterator seq in sequence over the TransformerIterator.

Patch: clj-1738-4.patch

Comment by Marshall T. Vandegrift [ 17/Jun/15 9:57 AM ]

Sorry, what just happened here? Is this no longer being fixed?

Comment by Alex Miller [ 17/Jun/15 10:06 AM ]

Hey Marshall, I thought you might have some questions. As noted above, Rich decided that this should not be a valid usage of seqs over these kinds of iterators (even though your usage happened to suffice in the past). So, you should alter your code to use these iterators in a different way.

Comment by Marshall T. Vandegrift [ 17/Jun/15 10:08 AM ]

Will there be a "breaking changes" section in the release notes for 1.7?

Comment by Alex Miller [ 17/Jun/15 11:20 AM ]

I will add a compatibility section. In this case, it should be considered "already broken" (but you're just now aware of it) I think.

Comment by Mike Rodriguez [ 17/Jun/15 10:09 PM ]

The main question I have is what is the proposed alternative way to interact with these object reusing iterators? I struggle to see what Clojure functions are safe to use on them because anything that internally calls seq must be avoided.

Comment by Alex Miller [ 18/Jun/15 5:23 AM ]

I would say that you shouldn't expect any sequence functions (all of which coerce to seq) to give you useful results. Instead, either consume the iterator in a loop/recur or create a lazy-seq that transforms each re-returned mutable object into a proper value prior to caching.

Comment by Marshall T. Vandegrift [ 18/Jun/15 7:11 AM ]

I expect at this point it isn't possible to change in Clojure/core's mind, but, Alex, your last comment crystallized my specific objection to this change.

You suggest "create a lazy-seq that transforms each re-returned mutable object into a proper value prior to caching." When the seq element-realization semantics match the element-at-a-time `Iterator` element-realization semantics there's an exact function for this: `map`. Specifically that this change breaks existing, working instances of the pattern `(map get-value iterable)` to me clearly demonstrates that the change is not compatible with the semantics of the `Iterator` interface. The fact that the newly-brokenness of the pattern is so non-obvious just emphasizes the point.

An unknown amount of deployed code in the Clojure ecosystem, and a non-trivial amount in my own code bases, are currently using this pattern to handle mutating-element Iterators. From my own tally of used Java-ecosystem libraries which include this pattern, I believe the mutating-Iterator case tmore common than Clojure/core apparently expect. For myself and all the other developers using Clojure to orchestrate large and obtuse Java frameworks, I plea for compatibility.

Comment by Alex Miller [ 18/Jun/15 8:32 AM ]

"When the seq element-realization semantics match the element-at-a-time `Iterator` element-realization semantics" makes an incorrect assumption. As a general guideline, code that relies on how many or when elements of a lazy seq are realized is fragile - Clojure does not make guarantees about that.

Additionally, there is another assumption that the iterator seq will only be traversed once, as you will get different and incorrect results after the first time.

Use of a seq built on this kind of iterator violated these assumptions, even if it happened to work.

Comment by Marshall T. Vandegrift [ 18/Jun/15 8:47 AM ]

I respectfully disagree.

"As a general guideline, code that relies on how many or when elements of a lazy seq are realized is fragile - Clojure does not make guarantees about that." This is true about seqs in general, but the Iterator interface does guarantee that a single element at a time is realized. I strong believe that a correct interop abstraction for generating seqs from Iterators must maintain this guarantee. I'm not making a claim about seqs in general, just for Iterator->seq coercion in order to maintain the semantics of the underlying Iterator and thus provide a useful & correct interop facility.

"Additionally, there is another assumption that the iterator seq will only be traversed once, as you will get different and incorrect results after the first time." Either seqs cache or they don't, yes? I don't believe it is coherent to argue both that mutation is incompatible due to caching and that `map`ing is incompatible because there might not be caching.

Comment by Alex Miller [ 18/Jun/15 8:56 AM ]

The issue is with iterators that return elements that aren't values. Seqs cache values. If you're not using values as elements, then you are outside the bounds of what is supported.

Comment by Marshall T. Vandegrift [ 18/Jun/15 9:40 AM ]

I agree that's their primary intent, but then why functions like `dorun`? For most of Clojure's history seqs have been the primary abstraction for composable iteration over linear collections. With Clojure 1.7 in particular introducing a variety of finer-grained abstractions, I agree this more sharply defines the primary/optimal use for seqs. But this shouldn't come at the cost of invalidating existing code which uses only public interfaces or introducing mismatches with fundamental host platform abstractions like Iterator.

Comment by Mike Rodriguez [ 18/Jun/15 10:28 AM ]

"This is true about seqs in general, but the Iterator interface does guarantee that a single element at a time is realized."

-
This part makes it look like seqs and the Iterator interface are not compatible with one another and Clojure is just pretending they can be.

-
Having a chunking behavior paired with Java Iterators is going to be unreliable because the caller hasn't had a chance to see intermediate elements as they were consumed from the Iterator.

I'm still having difficulty in trying to understand how to interop with an API. My particular case is the (very popular) Hadoop ReduceContextImpl$ValueIterator. I tend to agree with Clojure's strong stance on values and against mutable state like this iterator uses. However, Hadoop apparently has done this from a practical standpoint where the cost of a very large number of object allocations outweighed the cost of adding the mutable state complexity. In this case, Hadoop still did uphold the contract for an Iterator and it made sense for consumers to deal with it against that contract.
When this enters Clojure, we may wrongfully interact with this Iterator as a chunking seq, when it really is not going to be match.

I've been using Clojure for a few years now in my full-time work and this scares me only because I struggle to know what functions I call that may inadvertently "chunk" the Iterator I'm interop'ing with from Java. If I had more clarity on that issue, I may be more comfortable with this. I still don't think the Iterator iterface should really be treated as "chunkable" with by seq though.

Comment by Alex Miller [ 18/Jun/15 11:08 AM ]

There are two ways to interop with an iterator like this - consume it in a loop/recur, or wrap it in a lazy seq. The latter is more similar to whatever you were already doing, so you'd want something like:

(defn iter-seq [iter f]
  (if (.hasNext iter)
    (lazy-seq
      (cons (f (.next iter))
            (iter-seq iter f)))))

which applies a function f to convert from the mutable thing returned from iter to a value. Apply this before doing anything else, then use the result as a normal seq.

Example using the mutating-iterable in the clj-1738-4.patch and AtomicLong.get() as f:

(let [mi (mutating-iterable 10)
      iter (.iterator mi)
      s (iter-seq iter #(.get %))]
    (println s)    ;; (0 1 2 3 4 5 6 7 8 9)
    (println s))   ;; (0 1 2 3 4 5 6 7 8 9)

Again, the real problem here is having a seq that contains mutating objects instead of values. Chunking just exposes that as the problem. If you care about whether chunking happens, then something is wrong.

Comment by Mike Rodriguez [ 18/Jun/15 12:11 PM ]

Thanks Alex. I appreciate the feedback. I certainly think this is valuable and a technique that I will keep in mind to avoid these issues.
My current real-world issue is in my usage of the Hadoop Iterable that has the mutating Iterator behind it. I have currently be using a `reduce` over this Iterable.
In this case, I believe I am safe since `reduce` operates at a different abstraction than the seq abstraction. I believe that is still a correct way to deal with this, but let me know if I'm mistaken.

For completeness, I'd like to make one more point on this topic in regards to "If you care about whether chunking happens, then something is wrong" with respect to Iterators.

I do not think mutability is the only concern of the element-at-a-time contract of an Iterator. The Iterator interface can be used as a stream of elements. This stream allows the consumer to view an individual element at a time, and decide what to do from there - copy it, pull some (derived) value from it, etc.
e.g. The consumer may decide to just look at an individual field of that element and then not need the element at all anymore. A common case is to iterate over an Iterator of items to calculate some summary value. Perhaps the elements are very memory intensive and we do intend to try and fit multiple (to some n count of elements) into memory all at the same time.

My key point is that the Iterator interface leaves the decision of whether or not to hold references to the elements on successive next() calls to the consumer.
Clojure's seq on Iterators makes a decision for the consumer that they can handle having a chunk (e.g. 32 elements) consumed from the Iterator at once. This doesn't have to strictly be a problem of mutable state. This can be about other resource management issues - such as memory in my above example.

I could also see it being that the Iterator is providing elements to the consumer that hold open some resources while the element is being "looked at". When hasNext() is called the Iterator impl could decide to close old connections to resources used in past iterations.
The consumer does not get to have a chance to look at some of these elements at the time they are available anymore, due to the assumption that Clojure makes of being able to read from the Iterator in chunks prior to the consumer seeing the items.

Again, I think I agree that caching and chunking of seqs is at odds with the contract of Iterator. It is because of this, that I find it sneaky how Clojure may behave with them in these circumstances.
I suppose that a seq for Iterator is really only for a special class of Iterators, where there is no concern for holding a chunk in memory at a time or the resource usage to realize a chunk at a time when only a single element may be needed at that point. This is the reality of how laziness interacts with chunking already though for the most part - things will may be lazy, but not necessarily one element at a time lazy.

I certainly can see this change of behavior sneaking up and breaking libraries out there that are interoping with Java Iterators at this point though.

Comment by Marshall T. Vandegrift [ 18/Jun/15 12:14 PM ]

I do understand the available solutions. My dual concern is that they should not be necessary, and that it will not be immediately clear in existing code where the solutions need to be applied.

It seems that I'm not going to be able to convince you, and have no ability to even attempt to convince Rich etc. I'll probably go the route of using a internally-patched version where I want to upgrade existing applications to Clojure 1.7.

Even though we could not come to agreement, I appreciate the time you've taken discussing this issue and seeking resolution for it. Thank you!

Comment by Alex Miller [ 18/Jun/15 2:11 PM ]

While I think you can see the interface of iterators and seqs in a similar way, I perceive them very differently.

I see Java iterators as a stateful interface for external iteration of a source - that is, they provide a processing model. Being stateful, iterators are (in most cases) not safe to share across threads. Once you create one, you have to control access to it.

I see seqs as being a logical list data structure that may have a variety of strategies for production. Caching and immutability are very much bound up in that sense that seqs can be treated as data, passed around safely, etc even if they are built initially on demand.

The sequence you are producing from one of these iterators will give you different results if looked at more than once. This feels deeply wrong to me from a Clojure perspective - as a seq user this violates every conception I have. Yes, you could (previously) use that seq as a form of iteration, but imo that's an abuse of knowing too much about the implementation. If you care about allocation costs, then using a seq that creates and caches seq nodes is a waste of memory. If you care about resource management, laziness is also a bad fit for that need as it's difficult to know when a resource has been completed.

Instead of trying to wedge everything into sequences, consider your new options in 1.7! You could use an eduction to delay processing but eagerly process a stack of transformations without allocation on an iterable source when it's time to do so. Or transduce/into/etc to do it eagerly. Or even sequence to compute it incrementally, which is actually a better answer than the lazy-seq one I gave above. reduce does walk the iterator one-by-one (there's no other way to do it!), and will apply the reducing function to each element before obtaining the next, so using either sequence (if you want caching) or eduction (if you just want delay) or into or reduce/transduce all in combination with a map transducer that produces a value, is another good solution in 1.7:

user=> (def to-val #(.get %)) ;; mutable object to value 
#'user/to-val 
user=> (into [] (map to-val) (mutating-iterable 10)) 
[0 1 2 3 4 5 6 7 8 9] 
user=> (eduction (map to-val) (mutating-iterable 10)) 
(0 1 2 3 4 5 6 7 8 9) 
user=> (sequence (map to-val) (mutating-iterable 10)) 
(0 1 2 3 4 5 6 7 8 9)

All of those are just taking a single "value-convert" but could instead take an arbitrary composition of transducers instead (which will incur none of the intermediate seq node allocation vs using lazy seqs). So thanks for mentioning reduce Mike - those neurons hadn't connected in my head yet.

Comment by Mike Rodriguez [ 19/Jun/15 11:29 AM ]

After reading through your last response I can say I feel more comfortable about this change and the appropriate way to deal with these types of Iterators now.

I appreciate you going into all that detail to explain this. It looks like 1.7 has a lot to offer in allowing for these more "fine-grained" ways to interact with collections like this. +1 to that!

Comment by Ghadi Shayban [ 18/Jul/15 2:44 AM ]

Field report of this breaking: https://github.com/aphyr/tesser/blob/82f2c36915b036137b0e4d97aacebfa793db6b98/math/src/tesser/quantiles.clj#L101-L105

Comment by Mike Rodriguez [ 21/Dec/16 7:26 AM ]

I posted https://groups.google.com/forum/#!topic/clojure/ltU5VfBLcN4, which I think relates somewhat to the comment chain of this Jira. I didn't think it was directly the same issue necessarily though. Just FYI for anyone interested.





[CLJ-2088] 'into' can make maps from collection of lists, but vectors are ok. Created: 19/Dec/16  Updated: 20/Dec/16  Resolved: 19/Dec/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Eugene Aksenov Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug
Environment:

OS: Ubuntu 16.04, x86_64, Linux 4.2.0-42-generic



 Description   

'into' can make maps from collection of lists, but vectors are ok.

Good behavior:
(into {} [[:a "a"] [:b "b"]])
;;=> {:a "a", :b "b"}

(into {} '([1 "a"] [2 "b"]))
;;=> {1 "a", 2 "b"}

Bad examples:
(into {} ['(:a "a") '(:b "b")])
;;=> ClassCastException clojure.lang.Keyword cannot be cast to java.util.Map$Entry clojure.lang.ATransientMap.conj (ATransientMap.java:44)

(into {} [(list [:a "a"]) [:b "b"]])
;;=> ClassCastException clojure.lang.PersistentVector cannot be cast to java.util.Map$Entry clojure.lang.ATransientMap.conj (ATransientMap.java:44)

(into {} ['(1 "a") '(2 "b")])
;;=> ClassCastException java.lang.Long cannot be cast to java.util.Map$Entry clojure.lang.ATransientMap.conj (ATransientMap.java:44)

(into {} '('(1 "a") '(2 "b")))
;;=> ClassCastException clojure.lang.Symbol cannot be cast to java.util.Map$Entry clojure.lang.ATransientMap.conj (ATransientMap.java:44)



 Comments   
Comment by Alex Miller [ 19/Dec/16 11:53 AM ]

into is built on conj (takes a sequential collection of elements to invoke with conj). conj for maps takes a map entry - a subclass of java.util.Map$MapEntry or a 2-element vector. Both of these choices can access keys and vals directly without "traversing" the entry (going through the key to get to the value).

We don't want to support that for performance reasons, so lists or seqs or not valid as map entries (and conj on a map does not support them).

into takes a sequential collection of these entries though, so vector, or list, or seq are all fine as the src collection for into.

So, all of this is working as intended and I am declining the ticket.

Comment by Eugene Aksenov [ 20/Dec/16 2:09 AM ]

Ok, live and learn
I've just added this code case and brief explanation to clojuredocs.org. Hope no one will be confused anymore.





[CLJ-1951] bigint? predicate and generator Created: 08/Jun/16  Updated: 17/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: generator

Approval: Vetted

 Description   

Add bigint? and spec.gen support.

This part is easy:

(defn bigint?
  "Returns true if n is a BigInt"
  {:added "1.9"}
  [n] (instance? clojure.lang.BigInt n))

The generator is the tricky bit. test.check doesn't have a generator for bigints, just large-integer for things in long range. I think we'd want numbers beyond long range in a bigint generator (as that's a likely place where bugs might lie). Making a really high-quality bigint generator (with good growth and shrinking characteristics) is something that needs more thought.

http://clojure.github.io/test.check/clojure.test.check.generators.html#var-large-integer



 Comments   
Comment by Gary Fredericks [ 17/Dec/16 6:17 PM ]

In case I don't get around to making a patch, I think a generator along these lines would be a decent start:

(def gen-bigint 
  (gen/sized 
   (fn [size] 
     (let [large-integer (gen/resize size gen/large-integer)] 
       ;; scaling gives us relatively small vectors, but using  
       ;; the resized large-integer above means the numbers in 
       ;; the small vectors will still be big 
       (gen/scale #(+ 2 (/ % 20))
                  (gen/fmap (fn [xs] (+ (bigint (first xs)) (reduce * (map bigint (rest xs))))) 
                            (gen/not-empty (gen/vector large-integer))))))))
Comment by Gary Fredericks [ 17/Dec/16 6:21 PM ]

If anything seems inadequate about the sizing in the above generator, I should point out that sizing in test.check is rather subtle but the requirements are also not well-defined. I'd be happy to discuss in detail.





[CLJ-2087] map does not work with core.async/<! Created: 17/Dec/16  Updated: 17/Dec/16  Resolved: 17/Dec/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Roy Varghese Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

The construct
(map <! [chan1 chan2 chan3])

does not work for reasons outlined here https://github.com/clojure/core.async/wiki/Go-Block-Best-Practices

This leads to very obscure bugs.

If this cannot be fixed in map, please consider throwing an exception when it cannot handle the passed function.



 Comments   
Comment by Alex Miller [ 17/Dec/16 5:08 PM ]

It's unlikely that we're going to "fix" this as it part of the design of Clojure.

When I try your example I see:

AssertionError Assert failed: <! used not in (go ...) block




[CLJ-1293] Support (try .. (catch :default _ ..)) for portable "catch-all" Created: 05/Nov/13  Updated: 16/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 11
Labels: portability

Attachments: Text File CLJ-1293-v001.patch     Text File CLJ-1293-v002.patch     Text File CLJ-1293-v003.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Clojurescript has this to expose the untyped catch, which is equivalent to (catch Throwable _) on java.

http://dev.clojure.org/jira/browse/CLJS-661

Proposals

1) add (catch :default _) to mean (catch Throwable _)
2) add (catch :default _) to mean (catch Exception _)
3) add (catch :all _) to mean (catch Throwable _)

Please see design page for discussion of proposals: http://dev.clojure.org/display/design/Platform+Errors

Patches

v001 implements just 1)

This patch is more permissive than my patch for CLJS: The CLJS patch ensures :default catch blocks occur between non-default catch blocks and finally blocks, if present. This patch just makes (catch :default ...) a synonym for (catch Throwable ...). I wanted to keep the change to the compiler minimum.

Open Question: Catch Throwable (patch v001 does this) or Exception? Alternatively, a more carefully crafted list of "non-fatal" errors. See Scala's NonFatal pattern extractor: http://www.scala-lang.org/api/current/scala/util/control/NonFatal$.html

v002 implements 2) + 3)

This builds on v001, so the same caveat about clause ordering applies.

v003 implements just 2)

This builds on v001, so the same caveat about clause ordering applies.



 Comments   
Comment by Brandon Bloom [ 28/Dec/14 11:33 AM ]

Noticed this switched from "Minor" to "Critical", so I figured I should mention that I later realized that we might want :default to catch Exception instead of Throwable, so as to avoid catching Error subclasses. Javadocs say: "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch." If that's what we actually want, I can provide an updated patch.

Comment by Alex Miller [ 28/Dec/14 2:19 PM ]

Seems like an open question, might be best just to list it as such in the description.

I don't really expect to reach consensus on the ticket or patch right now, just trying to update priorities and raise visibility for discussion with Rich once we get to 1.8.

Comment by Herwig Hochleitner [ 07/Dec/16 4:15 PM ]

I'm in favor of catching Exception. It is the :default on java (as stated in the docs), so catching Throwable is a platform-specific thing to do and it would still be possible.

Comment by Herwig Hochleitner [ 07/Dec/16 5:08 PM ]

Hm, realizing now, that my last comment is at odds with the design discussion about being able to catch anything in javascript.

Attached patch v002 implements :all in addition to :default.

Comment by Herwig Hochleitner [ 07/Dec/16 6:30 PM ]

Realized, that catch-all vs catch-Exception is only a shallow contradiction: (catch Exception _) is - for all intents and purposes - the catch-all of java. Since the catch-absolutely-all is accessible in java through a regular catch, the driving need in clojurescript doesn't apply to clojure. The driving need in clojure is portability. In clojurescript, this is conflated with exposing an otherwise inaccessible platform feature, but that needs to not drive the general design.

Attached v003





[CLJ-2085] Add additional info to explain-data to help explain printers Created: 15/Dec/16  Updated: 15/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec

Attachments: Text File explain-data.patch    
Patch: Code
Approval: Vetted

 Description   

Right now, the explain-data provided to the explain printer function only has the list of problem data. There are many interesting things a printer could do with access to the root spec and value but those are not currently available.

Proposed: Provide these values in the explain-data map (as ::spec and ::value).

Patch: explain-data.patch






[CLJ-2077] Clojure can't be loaded from the boot classpath under java 9 Created: 06/Dec/16  Updated: 15/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: java9

Approval: Triaged

 Description   

As part of the changes for the jigsaw module system in Java 9, the
java packages available to the boot classloader are now a subset of
the the full java distribution. This means that classes loaded via the
boot classloader cannot access any classes outside of that subset.

The list of packages not visible to the boot classloader are:

java.activation
java.annotations.common
java.compact1
java.compact2
java.compact3
java.compiler
java.corba
java.scripting
java.se
java.se.ee
java.sql
java.sql.rowset
java.transaction
java.xml.bind
java.xml.ws
jdk.accessibility
jdk.charsets
jdk.crypto.ec
jdk.crypto.pkcs11
jdk.dynalink
jdk.jsobject
jdk.localedata
jdk.naming.dns
jdk.scripting.nashorn
jdk.xml.dom
jdk.zipfs
jdk.attach
jdk.compiler
jdk.hotspot.agent
jdk.internal.le
jdk.internal.opt
jdk.jartool
jdk.javadoc
jdk.jconsole
jdk.jdeps
jdk.jdi
jdk.jlink
jdk.jshell
jdk.jstatd
jdk.jvmstat

Clojure itself only uses one package on that list: java.sql. It is
used in clojure.instant to provide print-method and
print-dup implementations for java.sql.Timestamp, and in
clojure.core/resultset-seq.

This can be seen with (using Clojure 1.4.0 or higher, and a early-access build
of Java 9, most recently tested with 9-ea+147):

java -Xbootclasspath/a:clojure.jar clojure.main -e "(require 'clojure.instant)"

This affects any clojure-based tool that puts itself on the boot
classpath in order to gain a startup time boost (both lein
and boot are affected currently).



 Comments   
Comment by Toby Crawley [ 06/Dec/16 12:34 PM ]

More details on the underlying change that is triggering this are available at http://openjdk.java.net/jeps/261 (search for java.sql to find the relevant section).

Comment by Alex Miller [ 06/Dec/16 8:41 PM ]

Does this need to be a ticket here? Or is this really an issue for build tools?

Comment by Toby Crawley [ 08/Dec/16 4:30 PM ]

That depends on if we want using Clojure from the boot classpath to be an acceptable use case. If not, then I agree, it is just an issue for tooling.

Comment by Toby Crawley [ 09/Dec/16 2:21 PM ]

I realized today that this issue doesn't actually affect boot, since it doesn't use the bootclasspath. So lein is the only tooling I know of that is affected by this.

Comment by Ivan Pierre [ 12/Dec/16 4:59 AM ]

https://docs.oracle.com/javase/8/docs/api/java/sql/Timestamp.html
Would it be possible to use java.util.Date instead. Alas it's not possible to downcast :
Due to the differences between the Timestamp class and the java.util.Date class mentioned above, it is recommended that code not views Timestamp values generically as an instance of java.util.Date. The inheritance relationship between Timestamp and java.util.Date really denotes implementation inheritance, and not type inheritance.

Comment by Ivan Pierre [ 12/Dec/16 8:22 AM ]

The problem with Date is that it doesn't deal with nanoseconds. But Timestamp is created by a long giving the TimeDate value in milliseconds.
The use of setNano and getNano are only to store the SQL TIMESTAMP. Wouldn't it be better to deal with this value in another way?

The other way is to take just what we need from TimeStamp, and it's just a little thing, I'll try to compile with that to see if some other thing comes after...

Test code : https://gist.github.com/ivanpierre/b0ea937dac97d910a7c3c1e5774028e0

Comment by Ivan Pierre [ 12/Dec/16 1:13 PM ]

Ok, I pass to the GNU version of Timestamp. The code is neater. I mixed some of Sun's for more consistency. I dropped the string management of dates as Clojure will do it in clojure.instant.

It still works. I had a doubt...

If I type (clojure.lang.TimeStamp. 3678141) the response will be :
==> #inst "1970-01-01T01:01:18.141000000-00:00" with a nano of
141000000

But is if set nano to 1 : (.setNanos (clojure.lang.TimeStamp. 3678141) 1) the response is : #inst "1970-01-01T01:01:18.000000001-00:00"

This is correct, but it's a little disturbing to see my nice .141 disappear...

I put a fork on my GitHub. Last commit : https://github.com/ivanpierre/clojure/commit/749a0184ee7409290dad9ff353605fcaabd64f69

So, good, now pass to Leinigen...

Comment by Alex Miller [ 13/Dec/16 10:32 AM ]

I think the first question here is: do we expect that Clojure should be loadable from the bootclasspath?

While I realize this is a hack people use, my initial answer would be no (that was never a design constraint afaik). But will need to defer to Rich on that.

Comment by Ivan Pierre [ 13/Dec/16 10:41 AM ]

Well Leiningen do it, even with the lein repl. I test to see if TimeStamp is the only thing.
Alas Leinigen is not 1.9 compatible, so I have to go down to version 1.8 to make the tests. (problem of conflict between clojure.spec and hara library)
A funny thing would be to pass the whole Clojure test in bootstrap so we would know...

Comment by Ivan Pierre [ 13/Dec/16 1:06 PM ]

The java Timestamp could be directly integrated into clojure.instant as it's a new datatype. So no need to worry about copyright stuff, and integer it in a complete manner to accept SQL TIMESTAMP, and some correct protocols.
The worst is that looking across the DBs documentation doesn't help a lot and some are quite contradictory.





[CLJ-1239] faster, more flexible dispatch for clojure.walk Created: 29/Jul/13  Updated: 14/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 7
Labels: walk

Attachments: Text File 0001-CLJ-1239-protocol-dispatch-for-clojure.walk.patch     Text File 0002-CLJ-1239-protocol-dispatch-for-clojure.walk.patch    
Patch: Code
Approval: Triaged

 Description   

The conditional dispatch in clojure.walk is slow and not open to extension. Prior to CLJ-1105 it did not support records.

Approach: Reimplement clojure.walk using protocols. The public API does not change. Users can extend the walk protocol to other types (for example, Java collections) if desired. As in CLJ-1105, this version of clojure.walk supports records.

Patch: 0002-CLJ-1239-protocol-dispatch-for-clojure.walk.patch

Performance: My tests indicate this is 1.5x-2x the speed of the original clojure.walk. See https://github.com/stuartsierra/clojure.walk2 for benchmarks.

Risks: This approach carries some risk of breaking user code that relied on type-specific behavior of the old clojure.walk. When running the full Clojure test suite, I discovered (and fixed) some breakages that did not show up in clojure.walk's unit tests. See, for example, commit 730eb75 in clojure.walk2



 Comments   
Comment by Vjeran Marcinko [ 19/Oct/13 12:32 PM ]

It looks, as it is now, that walking the tree and replacing forms doesn't preserve original meta-data contained in data structures.

Comment by Andy Fingerhut [ 23/Nov/13 1:11 AM ]

Patch 0001-CLJ-1239-protocol-dispatch-for-clojure.walk.patch no longer applies cleanly to latest Clojure master since the patch for CLJ-1105 was committed on Nov 22, 2013. From the description, it appears the intent was either that patch or this one, not both, so I am not sure what should happen with this patch, or even this ticket.

Comment by Alex Miller [ 23/Nov/13 1:52 AM ]

This patch and ticket are still candidates for future release.

Comment by Stuart Sierra [ 20/Dec/13 9:14 AM ]

Added new patch that applies on latest master after CLJ-1105.

Comment by Chouser [ 27/Feb/14 10:26 AM ]

The way this patch behaves can be surprising compared to regular maps:

(clojure.walk/prewalk-replace {[:a 1] nil} {:a 1, :b 2})
;=> {:b 2}

(defrecord Foo [a b])
(clojure.walk/prewalk-replace {[:a 1] nil} (map->Foo {:a 1, :b 2}))
;=> #user.Foo{:a 1, :b 2}

Note how the [:a 1] entry is removed from the map, but not from the record.

Here's an implementation that doesn't suffer from that problem, though it does scary class name munging instead: https://github.com/LonoCloud/synthread/blob/a315f861e04fd33ba5398adf6b5e75579d18ce4c/src/lonocloud/synthread/impl.clj#L66

Perhaps we could add to the defrecord abstraction to support well the kind of things that synthread code is doing clumsily, and then walk could take advantage of that.

Comment by Alex Miller [ 27/Feb/14 2:11 PM ]

@Chouser, can you file a new ticket related to this? It's hard to manage work on something from comments on a closed ticket.

Comment by Alex Miller [ 27/Feb/14 3:54 PM ]

@Chouser - Never mind! I was thinking this was the change that went into 1.6. Carry on.

Comment by Nicola Mometto [ 27/Feb/14 5:17 PM ]

Alex, for what it matters clojure-1.6.0 after CLJ-1105 exibits the same behaviour as described by Chouser for this patch





[CLJ-308] protocol-ize with-open Created: 21/Apr/10  Updated: 14/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: io

Attachments: Text File 0001-Added-ClosableResource-protocol-for-with-open.patch    
Patch: Code
Approval: Triaged

 Description   

Good use (and documentation example) of protocols: make with-open aware of a Closable protocol for APIs that use a different close convention. See http://groups.google.com/group/clojure/browse_thread/thread/86c87e1fc4b1347c



 Comments   
Comment by Assembla Importer [ 24/Aug/10 4:39 PM ]

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

Comment by Tassilo Horn [ 23/Dec/11 5:11 AM ]

Added a CloseableResource protocol and extended it on java.io.Closeable (implemented by all Readers, Writers, Streams, Channels, Sockets). Use it in with-open.

All tests pass.

Comment by Tassilo Horn [ 23/Dec/11 7:14 AM ]

Seems to be related to Scopes (http://dev.clojure.org/jira/browse/CLJ-2).

Comment by Tassilo Horn [ 08/Mar/12 3:59 AM ]

Updated patch.

Comment by Andy Fingerhut [ 02/Apr/12 12:11 PM ]

Patch 0001-Added-ClosableResource-protocol-for-with-open.patch dated 08/Mar/12 applies, builds, and tests cleanly on latest master as of Apr 2 2012. Tassilo has signed a CA.

Comment by Tassilo Horn [ 13/Apr/12 11:23 AM ]

Updated patch to apply cleanly against master again.

Comment by Brandon Bloom [ 22/Jul/14 9:00 PM ]

I looked up this ticket because I ran in to a reflection warning: with-open does not hint it's binding with java.io.Closeable

Some feedback on the patch:

1) This is a breaking change for anyone relying on the close method to be duck-typed.

2) CloseableResource is a bit long. clojure.core.protocols.Closeable is plenty unambiguous.

3) Rather than extending CloseableResource to java.io.Closeable, you can use the little known (undocumented? unsupported?) :on-interface directive:

(defprotocol Closeable
  :on-interface java.io.Closeable
  (close [this]))

That would perform much better than the existing patch.

Comment by Tassilo Horn [ 23/Jul/14 7:12 AM ]

Hi Brandon, two questions:

Could 1) be circumvented somehow by providing a default implementation somehow? I guess the protocol could be extended upon Object with implementation (.close this), but that would give a reflection warning since Object has no close method. Probably one could extend upon Object and in the implementation search a "close" method using java.lang.reflect and throw an exception if none could be found?

Could you please tell me a bit more about the :on-interface option? How does that differ from extend? And how do I add the implementation, i.e., (.close this) with that option?





[CLJ-1527] Clarify and align valid symbol and keyword rules for Clojure (and edn) Created: 18/Sep/14  Updated: 14/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 16
Labels: reader

Approval: Triaged

 Description   

Known areas of under-specificity (http://clojure.org/reader#The%20Reader--Reader%20forms):

  • symbols (and keywords) description do not mention constituent characters that are currently in use by Clojure functions such as <, >, =, $ (for Java inner classes), & (&form and &env in macros), % (stated to be valid in edn spec)
  • keywords currently accept leading numeric characters which is at odds with the spec - see CLJ-1286

References:



 Comments   
Comment by Andy Fingerhut [ 17/Oct/14 2:13 AM ]

The Clojure reader documentation also does not mention the following symbols as valid constituent characters. They are all mentioned as valid symbol constituent characters in the EDN readme here: https://github.com/edn-format/edn#symbols

dollar sign - used in Clojure/JVM to separate Java subclass names from class names, e.g. java.util.Map$Entry
percent sign - not sure why this is part of edn spec. In Clojure it seems only to be used inside #() for args like % %1 %&
ampersand - like in &form and &env in macro definitions
equals - clojure.core/= and many others
less-than - clojure.core/< clojure.core/<=
greater-than - clojure.core/> clojure.core/>=

I don't know whether Clojure and edn specs should be the same in this regard, but it seemed worth mentioning for this ticket.

Comment by Andy Fingerhut [ 01/Jun/15 12:22 AM ]

Alex, Rich made this comment on CLJ-17 in 2011: "Runtime validation off the table for perf reasons. cemerick's suggestion that arbitrary symbol support will render them valid is sound, but arbitrary symbol support is a different ticket/idea." I am not aware of any tickets that propose the enhancement of allowing arbitrary symbols to be supported by Clojure, e.g. via a syntax like

#|white space and arbitrary #$@)$~))@ chars here|

Do you think it is reasonable to create an enhancement ticket for supporting arbitrary characters in symbols and keywords?

Comment by Alex Miller [ 01/Jun/15 6:36 AM ]

Sure. I looked into this a bit as a digression off of feature expressions and #| has been reserved for this potential use. However, there are many tricky issues with it and I do not expect this to happen soon - more likely to be something we're pushed to do when necessary for some other reason.

Comment by Herwig Hochleitner [ 01/Jun/15 8:46 AM ]

Wrong ticket, but to anybody thinking about #|arbitrary symbols (or strings)|, please do consider making the delimiters configurable, as in mime multipart.

Comment by Andy Fingerhut [ 01/Jun/15 8:54 AM ]

I've created a design page for now. I'm sure it does not list many of the tricky issues you have found. I'd be happy to take a shot at documenting them if you have any notes you are willing to share.

http://dev.clojure.org/pages/viewpage.action?pageId=11862058

Comment by Andy Fingerhut [ 01/Jun/15 9:01 AM ]

Herwig, can you edit the design page linked in my previous comment, to add a reference or example to precisely how mime multipart allows delimiters to be configurable, and why you believe fixed delimeters would be a bad idea?

Comment by Herwig Hochleitner [ 01/Jun/15 9:46 AM ]

I've commented on the design page.

Comment by Alex Miller [ 13/Jul/15 12:44 PM ]

Removed a couple of issues that have been clarified on the reader page and are no longer issues.

Comment by Nicola Mometto [ 13/Jul/15 12:45 PM ]

Related to CLJ-1530

Comment by Adam Frey [ 15/Jul/15 11:55 AM ]

Related to this: The Clojure reader will not accept symbols and keywords that contain consecutive colons (See LispReader.java), although that is permitted by the current EDN spec. Here is a GitHub issue regarding consecutive colons. I would like to qualify why consecutive colons are disallowed, and sync up the Clojure Reader and the EDN spec on this.

Comment by Herwig Hochleitner [ 31/Jul/15 8:03 AM ]

The updated reader spec says that a symbol can contain a single / to separate the namespace. It also mentions a bare / to be the division function.
So what about clojure.core//? That still got to be a readable symbol right? So is that an exception to the 'single /' rule?
Will foo.bar// also be readable? What about foo//bar?

Comment by Francis Avila [ 10/Sep/15 9:26 AM ]

Another source of ambiguity I see is that it's unclear whether the first colon of a keyword is the first character of the keyword (and therefore of the symbol) or whether it is something special and the spec really describes what happens from the second character onward. This matters because the specification for a keyword is (in both edn and reader specs) given in terms of differences from symbols. I think many of the strange keyword edge cases (including legality of :1 vs :a/1) stem from this ambiguity, and different tickets/patches seem to choose one or the other underlying assumption. See this comment for more examples.

Possibly we can use tagged literals for keywords and symbols to create or print these forms when they are not readable and simplify the reader spec for their literal forms. E.g. instead of producing complicated parse rules to ensure clojure.core// or :1 are legal, just make the literal form simple and have users write something like #sym["clojure.core" "/"] or #kyw "1" (and have the printer print these) when they hit these edge cases.

Comment by Alex Miller [ 10/Sep/15 9:44 AM ]

I would say : (and : are syntactic markers and the spec describes the characters following it. But I agree it would be nice for this to be more explicit. The (incorrect) regex in LispReader does not help either.

The tagged literal idea is an interesting alternative to the | | syntax that has been reserved for possible future support for invalid characters in keywords and symbols. But I think the idea is out of scope for this ticket, which is really about clarifying the spec.

Comment by Steven Yi [ 08/Nov/16 11:16 AM ]

Coming to this late, I had mentioned on the user mailing list in:

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

that # is currently allowed as part of a symbol name, such that:

(let a# 4 b#a 3 (println a# b#a))

will print "4 3".

  1. is also employed in auto-gensyms and discussed in http://clojure.org/reference/reader#syntax-quote as part of a symbol's name. From the mailing list thread, # was noted as "may be allowed now, but could be changed later". I would appreciate if it is more clearly described as a special case/reserved, and would ask that its use be restricted in the reader to prevent users from using it now and potentially have code break later.




[CLJ-1458] Enhance the performance of map merges Created: 04/Jul/14  Updated: 14/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: performance

Attachments: Text File 0001-very-simple-test-of-the-merge-function.patch     Text File clj-1458-4.patch     Text File CLJ-1458-5.patch     Text File CLJ-1458-6.patch     Text File CLJ-1458-7.patch     Text File CLJ-1458-transient-merge2.patch     Text File CLJ-1458-transient-merge3.patch     Text File CLJ-1458-transient-merge.patch     Text File merge-test-2.patch     File transient-merge.diff    
Patch: Code and Test
Approval: Triaged

 Description   

It would be nice if merge used transients.

Patch

  • clj-1458-7.patch

Approach
Migrate c.c/merge later in core after transients & reduce. Leave older version as merge1 for use in cases the precede the newer definition. Make APersistentMap/conj & ATransientMap/cons aware of IKVReduce.

The attached patch preserves two existing behaviors of merge

  • metadata propagation
  • the right hand side of the merges can be a Map.Entry, an IPersistentVector where size=2, and regular maps.

Screened by:



 Comments   
Comment by Jason Wolfe [ 13/Sep/14 5:09 PM ]

I will take a crack at a patch today.

Comment by Jason Wolfe [ 13/Sep/14 5:42 PM ]

This patch (transient-merge.diff) makes merge, merge-with, and zipmap (since it was right there and could obviously benefit from transients as well) use transients.

Three potential issues:

  • I had to move the functions, since they depend on transient and friends. I assume this is preferable to a forward declaration. This was the best place I could find, but happy to move them elsewhere.
  • I added multiple arities, to avoid potential performance cost of transient-ing a single argument. Happy to undo this if desired.
  • I had to slightly alter the logic in merge-with, since transient maps don't support contains? (or find).
Comment by Michał Marczyk [ 14/Sep/14 12:43 PM ]

I posted a separate ticket for zipmap, with patch, on 30/May/12: CLJ-1005.

Comment by Jason Wolfe [ 14/Sep/14 5:28 PM ]

Ah, sorry if I overstepped then. Happy to remove that change from this patch then if that will simplify things – just let me know.

Comment by Ghadi Shayban [ 28/Dec/14 10:07 PM ]

alternate approach attached delaying merge until after protocols load, and then using transducers.

Comment by Michael Blume [ 28/Dec/14 11:50 PM ]

Looks like you're doing (get m k) twice – shouldn't that be thrown in a local?

Comment by Michael Blume [ 29/Dec/14 1:41 PM ]

um, put, in a local, I mean, 'throw' was a bad choice of word.

Comment by Ghadi Shayban [ 29/Dec/14 2:14 PM ]

Yeah there's that – won't be using get anyways after CLJ-700 gets committed.

We should add performance tests too. merging two maps, three, many maps, also varying the sizes of the maps, and for merge-with, varying the % of collisions.

Need to go back to the (some identity) logic, otherwise metadata is propagated from maps other than the first provided. I'll fix later.

Comment by Michael Blume [ 29/Dec/14 2:49 PM ]

I don't know if this is supposed to be allowed, but this breaks

(merge {} [:foo 'bar])

which is used in the wild by compojure-api

Comment by Michael Blume [ 29/Dec/14 2:49 PM ]

https://github.com/metosin/compojure-api/blob/0.16.6/src/compojure/api/meta.clj#L198

Comment by Michael Blume [ 29/Dec/14 2:54 PM ]

Ghadi, contains? uses get under the covers, so it's still two gets, right? It seems like it'd be more performant to stick with the ::none trick.

Comment by Nicola Mometto [ 29/Dec/14 5:36 PM ]

This calls for if-let + find.

Comment by Ghadi Shayban [ 29/Dec/14 10:37 PM ]

new patch addressing concerns so far

Comment by Ghadi Shayban [ 29/Dec/14 10:48 PM ]

CLJ-1458-transient-merge3.patch removes silly inlining macro, uses singleton fns instead.

Comment by Michael Blume [ 29/Dec/14 11:14 PM ]

Nice =)

This should come with tests. If we want to preserve the ability to merge with a MapEntry, we should test it. This isn't so much a weakness of the patch as of the existing tests. I see merge and merge-with being used a few times in the test suite, but I see no test whose purpose is to test their behavior.

Comment by Michael Blume [ 29/Dec/14 11:17 PM ]

Extremely simple merge test, we need more than this, but this is a start

Comment by Alex Miller [ 22/Jun/15 10:11 AM ]

clj-1458-4.patch refreshed to apply to master, no changes.

Comment by Ghadi Shayban [ 09/Jan/16 5:09 PM ]

I'd like to reevaluate the scope of this ticket. Can we address 'merge' only and defer 'merge-with'? It's by far the more common function. I've attached a new simplified patch.

Comment by Ghadi Shayban [ 09/Jan/16 9:50 PM ]

CLJ-1458-6.patch is yet another cleaner approach

Comment by Alex Miller [ 01/Feb/16 5:17 AM ]

Can you update the ticket approach section to discuss the APersistentMap.cons / ASSOC changes. Also, can you add a before / after perf test for one or more common cases?

Comment by Michael Blume [ 28/Sep/16 1:55 PM ]

Updated patch to handle use of merge in core_print before it's defined in core

Comment by Ghadi Shayban [ 28/Sep/16 2:22 PM ]

If anyone wants to take stewardship of this, go ahead. I had trouble getting consistent performance improvements on this. Obviously this needs fresh benchmarks.

Comment by Alex Miller [ 28/Sep/16 2:28 PM ]

Yes, this needs a benchmark showing demonstrable improvement. The whole goal here is improved perf - if we can't prove it's consistently faster, then there is no point in even reviewing it.





[CLJ-2054] generator for `any?` occasionally generates `Double/NaN` for which equality semantics don't apply, and that is a problem for the :ret spec of many functions. Created: 07/Nov/16  Updated: 14/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Dimitrios Piliouras Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: generator, spec
Environment:

Ubuntu 16.10 - Oracle Java 8


Approval: Triaged

 Description   

The generator for `any?` will occasionally give back Double/NaN value(s). Since NaNs & equality (via `=`) don't get along, :ret spec'ing a fn which transforms/processes a collection according to a predicate, becomes rather problematic. That's because the most obvious thing to check under :ret (the case where the predicate didn't return true for any value, and so the output coll should be equal to the input coll because nothing was transformed/processed), cannot be expressed trivially.

The workaround I've come up with in my own specs is to spec the elements of the collection with `(s/and any? (complement double-NaN?))` instead of just `any?`, and it works. However, even though I can live without NaNs in the tests, I must admit it still feels sort of hacky.

Ideas:

1) The generator for `any?` could be hardcoded to never return Double/NaN. Sounds rather invasive.
2) The generator for `any?` could be reworked to somehow be configurable wrt allowing/prohibiting Double/NaNs. Then perhaps a dynamic-var and/or a macro (e.g. `without-NaNs`) could expose this (just brainstorming here).
3) The generator for `any? could stay as is, but a new equality operator could be added (e.g. `clojure.spec/===`), which somehow ignores NaNs (a naive implementation for instance might walk the data-structures and replace all NaNs with keywords, and only then perform a regular comparison).



 Comments   
Comment by Alex Miller [ 08/Nov/16 10:29 AM ]

Should consider whether this change is more appropriate in test.check or in the spec generator for any?.

Comment by Dimitrios Piliouras [ 11/Nov/16 12:31 PM ]

It turns out that my workaround does not fully work. I literally just stumbled in the following case:

{nil {[] {NaN 0}}}

which is a conforming value for:

(s/def ::persistent-map
(s/map-of ::anything-but-NaN ::anything-but-NaN)) ;; (s/and any? (complement double-NaN?))

So basically, the inner collections can still have NaNs. So far I've got 4 specs that I've written and faced this problem on all of them.





[CLJ-2068] s/explain of evaluated predicate yields :s/unknown Created: 23/Nov/16  Updated: 13/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 1
Labels: spec

Attachments: Text File clj-2068-2.patch     Text File clj-2068.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Got:

(s/explain #{1 2 3} 4)
val: 4 fails predicate: :clojure.spec/unknown

(s/explain odd? 10)
val: 10 fails predicate: :clojure.spec/unknown

Expected to receive a description of the failing predicate as in:

(s/def ::s #{1 2 3})
(s/explain ::s 4)
;; val: 4 fails spec: :user/s predicate: #{1 3 2}

(s/def ::o odd?)
(s/explain ::o 10)
val: 10 fails spec: :user/o predicate: odd?

Cause: specize was falling through on these cases to Object and just returning unknown.

Proposed:
Special handling for 2 cases:
1. Sets - explictly catch IPersistentSet and use the set as the form.
2. Functions - demunge the function name and use the qualified function name symbol as the form. Add a special check for anonymous functions and revert to ::unknown for those (not much we can do with an eval'ed anonymous function).

Patch: clj-2068-2.patch



 Comments   
Comment by Alex Miller [ 13/Dec/16 6:52 PM ]

Simplified anon fn check and added a few basic tests.





[CLJ-2084] Support MapEquivalence for defrecords Created: 13/Dec/16  Updated: 13/Dec/16  Resolved: 13/Dec/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: defrecord


 Description   

defrecord equality implies matching types. This has been discussed and decided upon such that people should only bother with defrecords when they want an equality partition. There are, however, use cases for defrecords as small structmaps to save memory and those require map-based equivalence. Clojure has a marker interface for map equivalence: clojure.lang.MapEquivalence, which gets used in = to decide whether something qualifies as a persistent map. This can be reused to change the equiv implementation of a defrecord.

Current Behavior

When MapEquivalence is implemented in a defrecord, = becomes non-reflexive

(= {} (->Rec)), (not= (->Rec) {})

Proposed Behavior

Add clojure.lang.MapEquivalence to implemented interfaces in a defrecord, to treat it as a PersistenMap in equality, disregarding the type tag.
APersistentMap.equiv already does this, defrecord implementation can call APersistentMap/mapEquals



 Comments   
Comment by Alex Miller [ 13/Dec/16 10:27 AM ]

Wouldn't this be a breaking change for anyone relying on records to compare not equals based on type?

Comment by Herwig Hochleitner [ 13/Dec/16 4:35 PM ]

The key phrase is "Add clojure.lang.MapEquivalence to implemented interfaces". Any existing defrecords would not be affected. Au contraire, having a defrecord implement MapEquivalence right now results in a non-reflective-equals clojure-gigo scenario, so it's completely uncharted territory and ripe for definition. Sorry for not being clearer in the description, would you consider reopening?

Comment by Alex Miller [ 13/Dec/16 5:15 PM ]

It's not uncharted territory. Records intentionally compare different for equality based on type.

If you don't want that, don't use records. Or pour your record into a map before comparing via into or select-keys.

Comment by Herwig Hochleitner [ 13/Dec/16 5:51 PM ]

Yes, and I'm not proposing to change a single bit of that, agreed?
I am proposing the possibility to have specific defrecords implement map-like equality by using an option that a) naturally fits with what's already there b) compiles but yields GIGO right now:

(defrecord Rec []
  clojure.lang.MapEquivalence)

How is this already charted? If it is, then this ticket needs to be refiled as a Defect, because then the current behavior makes no sense.

So, if you're not declining this on technical grounds, what is it then? If it is not wanting to add use cases to defrecords for philosophical reasons, you're using the term "breaking change" very loosely here. There can't be any existing expectations on defrecords implementing MapEquivalence right now, because such defrecords are invalid programs right now.
Are you arguing some sort of "general expectation" on defrecords? One that would stand without looking at specific definitions? What use would that be to anybody. Who works with a defrecord without having a general idea of what it definition is?

Comment by Herwig Hochleitner [ 13/Dec/16 6:19 PM ]

Just to be clear, I did my due diligence on this and I'm aware that the current equality situation for records is completely intentional, hence I'm proposing growing it. The argument might still be that we don't want to grow it in that particular direction, but it's definitely not breakage.

Comment by Alex Miller [ 13/Dec/16 6:34 PM ]

I thought you were asking to add MapEquivalence to all records, which would be a breaking change.

The docstring for defrecord says:

{pre}
In addition, defrecord will define type-and-value-based =,
and will defined Java .hashCode and .equals consistent with the
contract for java.util.Map.{pre}

The important thing there is "type-and-value-based", and that's part of the design of records. Asking for MapEquivalence on a map means value-based only. These two things are in conflict and thus I would agree that asking for MapEquivalence on a record makes no sense.

deftype exists so that you can make your own types with whatever equality/hash/behavior you want - if you want something like this, then build it on top of deftype.

Comment by Herwig Hochleitner [ 13/Dec/16 6:41 PM ]

yeah, changing defrecord to implement MapEquivalence by default would make no sense.

However, declaring a (specific) defrecord to be in the MapEquivalence partition makes perfect sense, doesn't it?
That way, we can type our records an structmap them too.

Comment by Herwig Hochleitner [ 13/Dec/16 6:49 PM ]

Anyway, I'll build this on deftype for data.xml in any case, because we want this to work on current and older clojure versions. I just thought it would fit neatly with clojure itself (since there is already a marker interface for that purpose, only its use currently leads to breakage) and wanted to make you aware of the idea. I'll recycle it through the mailing list, as soon as the deftype based thing is in data.xml





[CLJ-2083] spec for printable/readable/edn data Created: 12/Dec/16  Updated: 12/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

When spec'ing some things, I've used `any?` in a few cases where it is overly permissive. In particular, sometimes I need to specify a value must be printable/readable, such as when a value may wind up in an edn file. Similarly, I've needed to spec something must have non-generative value-identity, ie. ban closures, etc. Printable/readable or simply `edn?` would be a much better approximation than `any?`.



 Comments   
Comment by Brandon Bloom [ 12/Dec/16 1:08 PM ]

I realize that an edn? predicate would have O(N) runtime, vs an edn spec that could take advantage of every/every-kv etc for sampling conformance.

Comment by Herwig Hochleitner [ 12/Dec/16 11:12 PM ]

Related: CLJ-1527





[CLJ-2082] Improve documentation of clojure.walk/walk Created: 11/Dec/16  Updated: 11/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: David Cook Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docs, docstring, documentation


 Description   

The documentation for the clojure.walk module isn't clear on which methods recurse through data structures, and which only operate on the outermost layer. The documentation for clojure.walk/walk and clojure.walk/postwalk both use forms of the word "traverse," and there's nothing calling out that clojure.walk/walk, unlike the rest of the functions in the namespace, doesn't recurse through the provided form.






[CLJ-1943] clojure.spec should implicitly convert classes to specs Created: 03/Jun/16  Updated: 09/Dec/16  Resolved: 09/Dec/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Kevin Corcoran Assignee: Unassigned
Resolution: Declined Votes: 3
Labels: spec


 Description   

It would be nice if clojure.spec implicitly converted Java classes to specs, as it does for predicates. As a comparison, plumatic/schema allows classes to be used as schemas directly, and I take advantage of this regularly, as I currently use both schema and interop quite heavily.

For example, the spec guide contains the following:

(import java.util.Date)
(s/valid? #(instance? Date %) (Date.))  ;; true

... and then, later, defines:

(s/def ::date #(instance? Date %))

If classes were implicitly converted to specs, ::date would be unnecessary, and the first example could be simplified to:

(import java.util.Date)
(s/valid? Date (Date.))  ;; true

This would make clojure.spec a lot easier to use and adopt on my projects.



 Comments   
Comment by Alex Miller [ 04/Jun/16 9:07 PM ]

This was proposed and we decided not to include it in the initial release of spec. I do not know that we will in the future though, so leaving this open for now.

Comment by Sean Corfield [ 04/Jun/16 11:37 PM ]

At World Singles we use Expectations and it also automatically treats Java classes as type-based predicates. That said, I don't think a core library should do this. It's convenient "magic" but it doesn't actually feel very Clojure-y. I think I would vote against this being added to clojure.spec.

Comment by Alex Miller [ 09/Jun/16 9:03 AM ]

Note that for this particular example, inst? is now available in core.

Comment by Alex Miller [ 09/Dec/16 4:22 PM ]

We're not going to do this at the current time.





[CLJ-1940] spec has no way to specify a non-fn var should always conform Created: 30/May/16  Updated: 09/Dec/16  Resolved: 09/Dec/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Allen Rohner Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: spec


 Description   

It appears there's no way to specify that a non-function var should always conform, after e.g. alter-var-root or binding.



 Comments   
Comment by Alex Miller [ 05/Jun/16 3:20 PM ]

I'm not sure it makes sense to do this at all in the case of a def. If you really want to check it on definition you could do so by explicitly calling valid?.

If you want to check changes via alter-var-root, you can do so by setting a var validator using http://clojure.github.io/clojure/clojure.core-api.html#clojure.core/set-validator!

I again don't think it makes a lot of sense to do anything automatic in binding either. You can always validate it explicitly if you want to.

Basically, I think this is outside the use case spec is trying to cover but I'll check with Rich before declining.

Comment by Alex Miller [ 09/Dec/16 4:22 PM ]

I don't think we're going to add anything for this at the current time (but maybe it will be considered again in the future).





[CLJ-1998] clj.spec: improve boolean kw option naming Created: 03/Aug/16  Updated: 09/Dec/16  Resolved: 09/Dec/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Max Penet Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: spec


 Description   

We have a mix of boolean keyword options with and without trailing "?" at the moment. It would be good to settle to 1 style, hopefully the one with the trailing "?".

Ex: in map-of we have :conform-keys, in double-in: NaN? and :infinite? and possibly others.



 Comments   
Comment by Alex Miller [ 09/Dec/16 4:21 PM ]

I don't think we're going to make any changes for this, thanks





[CLJ-2081] for macro spec should know :let can't go in the first position Created: 09/Dec/16  Updated: 09/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: lvh Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 Description   

The for macro does not support :let in the first position. This was reported in CLJ-207 and a patch was produced but rejected. Since we have spec for macros now, it might be an opportunity to provide a better error message. (I think first-place let should be fine, but that's neither here nor there.)






[CLJ-2055] binding-form spec parses symbol-only maps incorrectly Created: 08/Nov/16  Updated: 09/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Defect Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

1.9.0-alpha14


Attachments: Text File CLJ-2055-01.patch    
Patch: Code
Approval: Screened

 Description   

The :clojure.core.specs/binding-form spec incorrectly treats some maps as sequential bindings.

Actual:

user=> (s/conform :clojure.core.specs/binding-form '{x y})
[:seq {:elems [[:seq {:elems [[:sym x] [:sym y]]}]]}]

Expected:

user=> (s/conform :clojure.core.specs/binding-form '{x y})
[:map {x y}]

Cause:

When there is no :keys, :strs, or :syms from :clojure.core.specs/map-special-binding, then :clojure.core.specs/seq-binding-form treats a map as sequential.

Proposed fix:

Include an (s/and vector? ...) check. See patch.

Patch: CLJ-2055-01.patch
Screened by: Alex Miller






[CLJ-2076] s/coll-of and s/map-of do not unform their elements Created: 05/Dec/16  Updated: 09/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File clj-2076-2.patch     Text File clj-2076.patch    
Patch: Code and Test
Approval: Vetted

 Description   

s/coll-of and s/map-of unform with identity but should unform their elements:

(s/def ::o (s/coll-of (s/or :i integer? :s string?)))
(->> [1 2 "blah"] (s/conform ::o) (s/unform ::o))
=> [[:i 1] [:i 2] [:s "blah"]]

Expected: [1 2 "blah"]

Cause: every-impl unform* just returns x

Approach: Use the init/add/complete fns to generate an unformed value (when needed)

Patch: clj-2076-2.patch



 Comments   
Comment by Alex Miller [ 07/Dec/16 5:50 PM ]

This needs tests and a bunch of verification, but first pass at fixing.

Comment by Alex Miller [ 09/Dec/16 8:03 AM ]

Added tests, ready to screen





[CLJ-2079] Generator overrides for spec aliases are not respected Created: 08/Dec/16  Updated: 08/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Defect Priority: Major
Reporter: Nate Smith Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: generator, spec

Approval: Vetted

 Description   

Generator overrides for spec aliases are not respected.

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(require '[clojure.spec :as s])
(require '[clojure.spec.gen :as gen])
(s/def ::original number?)
(s/def ::alias ::original)

(every? double? (gen/sample (s/gen ::alias {::alias gen/double})))
;; => false

Providing a generator override for the original spec works as expected:

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(every? double? (gen/sample (s/gen ::alias {::original gen/double})))
;; => true


 Comments   
Comment by Alex Miller [ 08/Dec/16 5:02 PM ]

Probably a missing delay in the alias case - there's another ticket that has the same cause.

Comment by Nate Smith [ 08/Dec/16 6:43 PM ]

Looks like it might be because gensub looks for matching overrides by calling spec-name, which returns the wrong value for spec aliases.

(require '[clojure.spec :as s])
(s/def ::original number?)
(s/def ::alias ::original)
(@#'clojure.spec/spec-name (s/get-spec ::alias))
;; => :user/original




[CLJ-2078] Add a maven profile to connect with an nrepl client Created: 07/Dec/16  Updated: 07/Dec/16  Resolved: 07/Dec/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: development

Attachments: Text File clj-2078-0.patch    
Patch: Code

 Description   

Opening an nrepl port into a project is a popular development strategy with clojure projects. It should also work with the clojure project.

Maven profiles make it possible to add development tools without affecting the rest of the build.

clojure-maven-plugin has this tool built right in as the clojure:nrepl goal and cider provides middlewares for many advanced ide functionalities.

Attached patch provides and documents a -P cider profile that lets you jack into a clojure source checkout.



 Comments   
Comment by Herwig Hochleitner [ 07/Dec/16 7:38 PM ]

Assuming that non-contrib covered code is OK for optional dev tools.

Comment by Alex Miller [ 07/Dec/16 8:44 PM ]

Thanks, I don't think we're going to do this.





[CLJ-2030] Auto-create alias namespaces Created: 28/Sep/16  Updated: 06/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.9

Type: Enhancement Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: keywords, namespace, portability, spec

Attachments: Text File clj-2030-2.patch     Text File clj-2030-3-1.patch     Text File clj-2030-3-2.patch     Text File clj-2030.patch    
Patch: Code and Test
Approval: Screened

 Description   

It is useful to name keywords in namespaces, without creating or requiring those namespaces. When wanting to do that via an ::alias/keyword, the aliased namespace has to actually exist, in order to be aliased.
Currently, in Clojure, this can be achieved dynamically, through a combination of create-ns and alias, Clojurescript requires a dummy file and a :require :as.

Proposals:

1. Extend clojure.core/alias to auto-create missing namespaces
2. Extend clojure.core/alias to accept varargs & {:as kvs}
3. Extend ns to accept (:alias ...) clauses

Patch:

  • clj-2030.patch does 1 (but not 2 or 3), and was screened by SDH
  • clj-2030-2.patch does 1+2 (but not 3)
  • clj-2030-3-1.patch does 1+3 (but not 2)
  • clj-2030-3-2.patch does 1+2+3

Before:

user=> (alias 'parts 'company.domain.parts)
java.lang.Exception: No namespace: company.domain.parts found

After:

user=> (alias 'parts 'company.domain.parts)
nil
user=> ::parts/widget
:company.domain.parts/widget


 Comments   
Comment by Alex Miller [ 28/Sep/16 10:04 PM ]

From original description:

My use case is a simplification of data.xml, which would benefit greatly from a uniform way to alias + auto-create namespaces within the ns clause.

I would like to support the syntax:

(ns foo.bar
  (:alias xh  #xml/ns "http://..<xhtml>.."
          svg #xml/ns "http://..<svg>.."))

{:tag ::xh/div
 :content [{:tag ::svg/g}]}

see https://github.com/bendlas/data.xml/commit/22cbe21181175d302c884b4ec9162bd5ebf336d7

Comment by Alex Miller [ 28/Sep/16 10:08 PM ]

Thanks for filing this, it is something we've looked at a bit already. I simplified the description a bit and moved the use case and syntax to comments. I don't really understand the ns :alias example given in your syntax proposal but I think it's very unlikely we would go that far.

Comment by Herwig Hochleitner [ 29/Sep/16 3:55 AM ]

My syntax example could already be implemented, if alias had the proposed behavior and was available in an ns clause. In the linked commit, I implemented a scheme to encode xml namespaces in clojure namespaces, by using percent-encoding. I could easily provide that reader tag, if clojure and clojurescript provided the proposed extensions to alias and ns.

Comment by Alex Miller [ 29/Sep/16 8:53 AM ]

Yeah, I get it now (sleep!). I think the particular example is distracting to understand the enhancement request though.

Comment by Alex Miller [ 18/Oct/16 9:13 AM ]

moving this to vetted just so we don't lose track of it, but Rich has not actually ok'ed this for 1.9 yet

Comment by Herwig Hochleitner [ 06/Dec/16 10:50 PM ]

added patches for 3





[CLJ-1912] Optimized version of the '<' and '>' functions for arties larger than 2 Created: 08/Apr/16  Updated: 03/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Anton Harald Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: performance

Attachments: Text File clj-1912.patch    
Patch: Code
Approval: Prescreened

 Description   

When looking at the code of the build-in functions '<' and '>', (next more) is invoked twice in each comparison of two neighboring arguments.

Proposed: Compute (next more) only once per 'iteration' for the following set of functions that have the same code pattern: =, ==, <, <=, >, >=.

Perf improvements (see comments for more details):

Function Arity Before After % improved
< 4 140.7 ns 141.6 ns -0.6%
< 10 505.3 ns 460.3 ns 8.9%
< 100 9.0 µs 8.6 µs 4.4%
< 10000 885 µs 851 µs 3.8%
= 4 86.1 ns 86.8 ns 0.8%
= 10 333.4 ns 300.6 ns 9.8%
= 100 4.28 µs 3.65 µs 14.7%
= 10000 397.4 µs 353.3 µs 11.0%
== 4 138.1 ns 135.7 ns 1.7%
== 10 487.9 ns 460.9 ns 5.5%
== 100 5.58 µs 5.27 µs 5.6%
== 10000 565.0 µs 537.7 µs 4.8%

Patch: clj-1912.patch

Prescreened by: Alex Miller



 Comments   
Comment by Alex Miller [ 08/Apr/16 4:23 PM ]

I don't think there is a particular reason, feel free to make a patch.

Comment by Jozef Wagner [ 01/Dec/16 2:19 PM ]

Patch added. As if-let is defined later in the file, a combination of let and if is used.

Comment by Alex Miller [ 01/Dec/16 8:22 PM ]

Can you include benchmark code and benchmarks?

Comment by Jozef Wagner [ 02/Dec/16 3:25 AM ]

Benchmarks for <

(bench (< 1 2 3 4)) ;; 140.726376 ns
(bench (new< 1 2 3 4)) ;; 141.661964 ns

(bench (< 1 2 3 4 5 6 7 8 9 10)) ;; 505.381596 ns
(bench (new< 1 2 3 4 5 6 7 8 9 10)) ;; 460.331840 ns

(bench (apply < (range 100))) ;; 9.020666 µs
(bench (apply new< (range 100))) ;; 8.604638 µs

(bench (apply < (range 10000))) ;; 885.361898 µs
(bench (apply new< (range 10000))) ;; 851.344031 µs

Benchmarks for =

(bench (= 1 1 1 1)) ;; 86.114371 ns
(bench (new= 1 1 1 1)) ;; 86.874012 ns

(bench (= 1 1 1 1 1 1 1 1 1 1)) ;; 333.438530 ns
(bench (new= 1 1 1 1 1 1 1 1 1 1)) ;; 300.628516 ns

(bench (apply = (repeat 100 1))) ;; 4.282752 µs
(bench (apply new= (repeat 100 1))) ;; 3.650438 µs

(bench (apply = (repeat 10000 1))) ;; 397.401808 µs
(bench (apply new= (repeat 10000 1))) ;; 353.294723 µs

Benchmarks for ==

(bench (== 1 1 1 1)) ;; 138.162620 ns
(bench (new== 1 1 1 1)) ;; 135.759047 ns

(bench (== 1 1 1 1 1 1 1 1 1 1)) ;; 487.963993 ns
(bench (new== 1 1 1 1 1 1 1 1 1 1)) ;; 460.982411 ns

(bench (apply == (repeat 100 1))) ;; 5.587064 µs
(bench (apply new== (repeat 100 1))) ;; 5.273621 µs

(bench (apply == (repeat 10000 1))) ;; 565.031286 µs
(bench (apply new== (repeat 10000 1))) ;; 537.789795 µs

Benchmark code





[CLJ-1952] include var->sym in clojure.core Created: 08/Jun/16  Updated: 02/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File clj-1952.patch     Text File clj-1952-v2.patch    
Patch: Code
Approval: Prescreened

 Description   

A lot of libraries define their own variant of `var->sym`, clojure.spec recently did so aswell as a private var called `->sym`.

This ticket proposes to move it from `clojure.spec` to `clojure.core` as a public var named `var->sym` and to refactor the two variants to use it.

Patch: clj-1952-v2.patch
Prescreened by: Alex Miller



 Comments   
Comment by Jozef Wagner [ 01/Dec/16 1:38 PM ]

Added patch

Comment by Alex Miller [ 01/Dec/16 7:58 PM ]

Patch has whitespace warnings that should be removed.

Comment by Jozef Wagner [ 02/Dec/16 2:10 AM ]

Attached updated patch clj-1952-v2.patch that has no trailing whitespaces.





[CLJ-840] Add a way to access the current test var in :each fixtures for clojure.test Created: 21/Sep/11  Updated: 02/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Hugo Duncan Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: clojure.test

Attachments: File add-test-var.diff     File clj840-20161122.diff     File clj840-2.diff    
Patch: Code
Approval: Triaged

 Description   

When looking at (log) output from tests written with clojure.test, I would like to be able to identify the output associated with each test. A mechanism to expose the current test var within an :each fixture would enable this.

One mechanism might be to bind a test-var var with the current test var before calling the each-fixture-fn in clojure.test/test-all-vars.

Patch: clj840-20161122.diff



 Comments   
Comment by Stuart Sierra [ 07/Oct/11 4:33 PM ]

Or just pass the Var directly into the fixture. Vars are invokable.

Comment by Hugo Duncan [ 07/Oct/11 4:45 PM ]

I don't think that works, since the the function passed to the fixture is not the test var, but a function calling test-var on the test var.

Comment by Hugo Duncan [ 21/Oct/11 10:34 PM ]

Patch to add test-var

Comment by Stuart Sierra [ 25/Oct/11 6:04 PM ]

*testing-vars* already has this information, but it's not visible to the fixture functions because it gets bound inside test-var.

Perhaps the :each fixture functions should be called in test-var rather than in test-all-vars. (The namespace of a Var is available in its metadata.) But then we have to call join-fixtures inside test-var every time.

Comment by Stuart Sierra [ 25/Oct/11 6:26 PM ]

Try this patch: clj840-2.diff.

This makes *testing-vars* visible to :each fixture functions, which seems intuitively more correct.

BUT it slightly changes the behavior of test-var, which I'm less happy about.

Comment by Hugo Duncan [ 25/Oct/11 8:07 PM ]

Might it make sense to provide a function on top of testing-vars to return the current test-var?

Comment by Stuart Sierra [ 28/Oct/11 9:14 AM ]

No, that function is first

Comment by Hugo Duncan [ 28/Oct/11 11:31 AM ]

I agree with having the dynamic vars as part of the extension interface, but would have thought that having a function for use when writing tests would have been cleaner. Just my 2c.

Comment by Andy Fingerhut [ 23/Nov/13 12:42 AM ]

With a commit made on Nov 22, 2013, patch clj840-2.diff no longer applies cleanly to latest master. Updating it appears like it might be straightforward, but best for someone who knows this part of the code well to do so.

Comment by Joe Littlejohn [ 22/Nov/16 11:30 AM ]

I'd find it very useful if this one was fixed.

I've added an updated patch that has the same content as clj840-2.diff but applies against the current master (c0326d2), as of November 22nd 2016.

Comment by Joe Littlejohn [ 28/Nov/16 5:59 AM ]

I realise I've only translated a patch provided by someone else here, but if there's anything further you think this one needs before it's in a fit state to be considered then please do shout and I'll endeavour to add something further. Thanks.

Comment by Alex Miller [ 29/Nov/16 8:56 AM ]

If you could update the ticket to better describe the approach of the patch that would be helpful.

Comment by Joe Littlejohn [ 02/Dec/16 9:08 AM ]

The proposed patch (clj840-20161122.diff) allows 'each' fixtures to access the var associated with the currently executing test by using (first *testing-vars*). As a result of this change, each fixtures are able to access the metadata associated with the current test var, including the name.

The patch achieves the above by changing the order in which functions are wrapped when a test and its associated 'each' fixtures are run. Before this patch, 'each' fixtures were combined into a single higher-order function, which was then given a thunk containing an invocation of the test-var function to execute as its body. After this patch, the test-var function is now responsible for joining and executing 'each' fixtures but, importantly, it does so within the scope of the binding expression that adds the current test var to *testing-vars*. test-var now invokes the joined fixtures function, rather than the joined fixtures function being given a think that invokes test-var.

Hopefully that's clear, ish





[CLJ-2073] AOT compilation can result in spurious ClassCastException during compile Created: 02/Dec/16  Updated: 02/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7, Release 1.8
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Paul Mooser Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: aot, compiler
Environment:

java version "1.8.0_112"
Java(TM) SE Runtime Environment (build 1.8.0_112-b16)
Java HotSpot(TM) 64-Bit Server VM (build 25.112-b16, mixed mode)


Attachments: File consumer.clj     File implementer.clj     File protocol.clj    

 Description   

If you try to compile the attached files as follows (assuming they are in "src"):

java -Dclojure.compile.path=out -cp "./clojure-1.8.0.jar:out:src" clojure.lang.Compile implementer protocol consumer

an exception will be thrown:

Exception in thread "main" java.lang.ClassCastException: implementer.Obj cannot be cast to protocol.Dependent, compiling:(consumer.clj:5:1)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3657)
	at clojure.lang.Compiler.compile1(Compiler.java:7474)
	at clojure.lang.Compiler.compile(Compiler.java:7541)
	at clojure.lang.RT.compile(RT.java:406)
	at clojure.lang.RT.load(RT.java:451)
	at clojure.lang.RT.load(RT.java:419)
	at clojure.core$load$fn__5677.invoke(core.clj:5893)
	at clojure.core$load.invokeStatic(core.clj:5892)
	at clojure.core$load.doInvoke(core.clj:5876)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invokeStatic(core.clj:5697)
	at clojure.core$compile$fn__5682.invoke(core.clj:5903)
	at clojure.core$compile.invokeStatic(core.clj:5903)
	at clojure.core$compile.invoke(core.clj:5895)
	at clojure.lang.Var.invoke(Var.java:379)
	at clojure.lang.Compile.main(Compile.java:67)
Caused by: java.lang.ClassCastException: implementer.Obj cannot be cast to protocol.Dependent
	at protocol$fn__12$G__8__14.invoke(protocol.clj:3)
	at protocol$fn__12$G__7__17.invoke(protocol.clj:3)
	at protocol$expand_deps.invokeStatic(protocol.clj:8)
	at protocol$expand_deps.invoke(protocol.clj:6)
	at clojure.lang.AFn.applyToHelper(AFn.java:154)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3652)
	... 15 more
  • This does not occur with 1.6 or earlier versions
  • This does not occur if you do not try to invoke AOT
  • This may not occur for some orderings of the arguments

This appears to be related to the class being loaded by two different class loaders, and also may result in the namespace being compiled more than once. This issue has popped up for us multiple times in our production build, but it took a while to realize it was a compiler issue and to find a minimal example.






[CLJ-1917] internal-reduce extended on StringSeq calls `.length` on every iteration step Created: 24/Apr/16  Updated: 01/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Dimitrios Piliouras Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: performance
Environment:

n/a


Attachments: Text File clj-1917.patch    
Patch: Code
Approval: Prescreened

 Description   

internal-reduce extended on StringSeq calls `.length` (on the same String object) upon every iteration step [1]. There is absolutely no need for this as the length of a String cannot change. Therefore, it can be bound once (in the `let` a couple of lines up) and used thereafter.

[1]: https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/protocols.clj#L151

Prescreened by: Alex Miller



 Comments   
Comment by Jozef Wagner [ 01/Dec/16 2:00 PM ]

Patch attached





[CLJ-2072] Primitive type aliases do not always work due to meta data evaluation Created: 01/Dec/16  Updated: 01/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Volkert Oakley Jurgens Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: typehints


 Description   

Primitive alias do not work when the meta data is evaluated, for example in the case of def.

In this example, char is interpreted to be the function char rather than the type alias. This is because clojure.lang.Compiler$DefExpr$Parser.parse evaluates the meta data on the symbol.

(def ^char x \space)
(String/valueOf x)
=> CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: clojure.core$char@7b82f7d1

Instead, this has to be written as

(def ^Character/TYPE x \space)

However, when using primitive type hints in-line, they work fine:

(def x \space)
(String/valueOf ^char x)

Primitive type aliases should be handled consistently.

Googling shows that this seems to be a well-known problem, but I have not found a Jira issue for it.



 Comments   
Comment by Alex Miller [ 01/Dec/16 7:48 PM ]

Var meta is evaluated. Meta on function signatures and other locations is not. Those two things are that way for historical reasons but would at this point be breaking changes if we changed either of them, so they are not going to change.

One thing that could potentially be done is to detect this particular problem when it happens and create a warning or error. In particular, this would present as a var whose meta :tag is a function.

So, if you want to re-write this ticket as a request for error message, that is something worth considering.





[CLJ-1955] .hashCode throws ClassCastException when called on some functions Created: 09/Jun/16  Updated: 01/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Georgi Danov Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File clj-1955.patch    
Patch: Code
Approval: Triaged

 Description   
user> some?
#function[clojure.core/some?]
user> (.hashCode map)
72400056
user> (.hashCode str)
ClassCastException clojure.core$str cannot be cast to java.lang.String  /eval39172 (form-init3428514420830954023.clj:5793)
user> (.hashCode (fn []))
1715179801
user> (.hashCode some?)
ClassCastException clojure.core$some_QMARK_ cannot be cast to java.lang.Boolean  /eval39178 (form-init3428514420830954023.clj:5797)
user> (.hashCode #'some?)
1955712430
user> (.hashCode @#'some?)
1726569843


 Comments   
Comment by Nicola Mometto [ 10/Jun/16 3:27 AM ]

This happens because `some?` and `str` have type hints on the Var to signal the type returned by their invocations, but the Compiler thinks those type hints apply to the Var object itself aswell.

An easy fix would be to move those type hints from the Var (old-style) to the argvec (new-style)

Comment by Ghadi Shayban [ 14/Jun/16 3:36 PM ]

agreed with nicola's suggestion - change type hints. This is a dup of CLJ-140 where :tag causes confusion when a var is being invoked vs used in expr context

Comment by Jozef Wagner [ 01/Dec/16 1:29 PM ]

Patch attached





[CLJ-1898] Inconsistent duplicate check in set/map literals with quoted/unquoted equal constants Created: 06/Mar/16  Updated: 01/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections, compiler

Attachments: Text File clj-1898.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Set and map literals containing the same constant quoted and unquoted, will throw a duplicate key exception in some cases (the correct behaviour), while silently ignore the duplicate in some others.

user=> #{'1 1}
#{1}
user=> #{'[] []}
IllegalArgumentException Duplicate key: []  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:56)

This happens because the compiler assumes that literals that have distinct elements at read-time, will have distinct elements at runtime. This is not true for self-evaluating elements where (quote x) is equal to x



 Comments   
Comment by Jozef Wagner [ 01/Dec/16 3:38 PM ]

Attached patch with tests.





[CLJ-2037] specs in registry lack :file metadata despite having :line, :column Created: 08/Oct/16  Updated: 01/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Felix Andrews Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 Description   

As of 1.9.0-alpha13, specs in the registry lack :file metadata despite having :line, :column

user=> (require '[clojure.spec :as s])
user=> (-> (s/registry) (get :clojure.core.specs/arg-list) (meta))
{:line 1118, :column 5, :clojure.spec/name :clojure.core.specs/arg-list}
user=> (-> (s/registry) (get 'clojure.core/let) (meta))
{:line 1675, :column 5, :clojure.spec/name clojure.core/let}

This would be useful because:

  • we could list all the specs defined in a project, by filtering the registry.
  • we could read the source of a spec, like clojure.repl/source, for pretty formatting.

(specifically, for use in Codox https://github.com/weavejester/codox/pull/134 )

I had a quick look but couldn't see where the metadata is set.
Cheers



 Comments   
Comment by Alex Miller [ 08/Oct/16 11:12 AM ]

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

Comment by Felix Andrews [ 12/Oct/16 11:29 PM ]

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.

Comment by Jozef Wagner [ 01/Dec/16 12:31 PM ]

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





[CLJ-2071] Unexpected behavior with clojure.spec/tuple and clojure.spec.test/instrument Created: 01/Dec/16  Updated: 01/Dec/16  Resolved: 01/Dec/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Ben Rady Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Clojure 1.9, JDK 8



 Description   

It looks like stest/instrument is comparing a sequence of actual args to a vector created by spec/tuple and it doesn't match, however clojure.spec.test/instrument appears to work fine. Reading the clojure.spec guide I would think the two approaches would be equivalent.

(ns sample
  (:require [clojure.spec.test :as stest]
            [clojure.spec :as spec]))

    (defn myinc [i]
      (inc i))

    (spec/fdef myinc
               :args (spec/tuple integer?) ; Fails with the error below
               ; :args (spec/cat :i integer?) ; This works
               :ret integer?)

    (stest/instrument `myinc)
    (myinc 1)
    ; clojure.lang.ExceptionInfo: Call to #'specific.core-spec/myinc did not conform to spec:
    ; val: (1) fails at: [:args] predicate: vector?
    ; :clojure.spec/args  (1)
    ; :clojure.spec/failure  :instrument
    ; :clojure.spec.test/caller  {:file "core_spec.clj", :line 28, :var-scope specific.core-spec/fn--6046}


 Comments   
Comment by Ben Rady [ 01/Dec/16 8:11 AM ]

Note that the namespace in this example was changed. It used to be specific.core-spec, which is shown in the error.

Comment by Alex Miller [ 01/Dec/16 10:22 AM ]

Spec will create a list or a seq of the args for checking the :args spec. Tuples can only be used on vectors (because they match by index). So, this is not currently expected to work. It is recommended that you use a regex spec (s/cat) instead.





[CLJ-2070] Faster clojure.core/delay implementation Created: 29/Nov/16  Updated: 30/Nov/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance
Environment:

macOS Sierra
intel Core i7 2.5G Hz
Memory 16GB

java version "1.8.0_66"
Java(TM) SE Runtime Environment (build 1.8.0_66-b17)
Java HotSpot(TM) 64-Bit Server VM (build 25.66-b17, mixed mode)


Attachments: Text File fast_delay_with_volatile_fn2.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

clojure.lang.Delay uses a synchronized lock to protect the deref method, because it must make sure the Delay object is realized exactly once.

As we known synchronized lock plays worse performance under heavy concurrency. Instead, using volatile and double-checking lock in this situation improves the performance. The benchmark code is at test-delay.clj. The benchmark-delay function accepts thread count number as an argument, and it will start as many threads as the argument to access delay object concurrently as in (time (benchmark-delay 1)).

threads 1.9.0-alpha14 + patch % better
1 0.570196 ms 0.499905 ms 12
10 19.66194 ms 1.313828 ms 93
20 40.740032 ms 2.149794 ms 95
100 184.041421 ms 8.317295 ms 95

Patch: fast_delay_with_volatile_fn2.patch
Prescreened by: Alex Miller



 Comments   
Comment by Alex Miller [ 29/Nov/16 8:52 AM ]

A faster version of delay would be helpful - these are used extensively inside spec so would actually help the average case spec performance.

These whitespace errors need to be cleaned up...

$ git apply ~/Downloads/fast_delay.patch
/Users/alex/Downloads/fast_delay.patch:67: trailing whitespace.
	                try
/Users/alex/Downloads/fast_delay.patch:105: trailing whitespace.

/Users/alex/Downloads/fast_delay.patch:115: space before tab in indent.
        	    (fn []
/Users/alex/Downloads/fast_delay.patch:116: space before tab in indent.
          		    (.await barrier)
/Users/alex/Downloads/fast_delay.patch:117: space before tab in indent.
          		    (dotimes [_ 10000]
warning: squelched 8 whitespace errors
warning: 13 lines add whitespace errors.

More importantly, the double-check is on fn, so it's critical that fn is marked as volatile. You should re-run the perf test afterwards.

Comment by dennis zhuang [ 29/Nov/16 9:13 AM ]

Sorry, white spaces errors should be fixed before my attached.

But the fn doesn't need to be marked as volatile, because it's protected by synchronized in all blocks. And writing it to be null is fine here.

fn=null;

It's not like double-checking in singleton example, there is no reordering here.

Comment by Alex Miller [ 29/Nov/16 9:25 AM ]

fn is read at the top before the synchronized block - it needs to be volatile or one thread may not see the write inside the synchronized block from another thread.

Comment by dennis zhuang [ 29/Nov/16 9:41 AM ]

Yep ,but it's fine here.
If one thread can't see the writing null for fn at the top, it will enter the locking block.
The double-checking on fn!=null will make sure the fn is called at most once, and if the fn was called by another thread and was set to be null ,then current thread will fail on second checking on fn!=null and exit the locking to go on reading value or exception.

So, in the worst situation, maybe two or more threads would enter the locking block,but they all will fail on second checking on fn!=null except one thread of them success.

I don't want to declare fn to be volatile, because volatile also has a cost on reading. The fn variable may be flushed into main memory too late, but it's acceptable and safe here, and we avoid the cost of volatile reading.

Comment by Alex Miller [ 29/Nov/16 9:45 AM ]

I think you're wrong, and I'm not screening it without it being marked as volatile.

Comment by dennis zhuang [ 29/Nov/16 9:54 AM ]

The patch which mark fn volatile.

Comment by dennis zhuang [ 29/Nov/16 9:54 AM ]

The patch which does't mark fn volatile.

Comment by dennis zhuang [ 29/Nov/16 9:59 AM ]

Hi, alex.

I understand your opinion here. Though i still insist that fn doesn't need to be marked as volatile, but it's not a critical concern here.

I uploaded two patches, one is marked fn volatile, the other is not. All of them fixed the whitespace errors and update the benchmark result in ticket description.

Thanks.

Comment by dennis zhuang [ 29/Nov/16 10:15 AM ]

Rebase master.

Comment by Nicola Mometto [ 30/Nov/16 11:53 AM ]

dennis, here's an article describing why fn needs to be volatile: https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

Comment by dennis zhuang [ 30/Nov/16 6:01 PM ]

@Nicola

I knew the double-checking issue in old JVM memory model, but it is different here.
There is no instance constructed, it's only assigning fn to be null, so it doesn't have instruments reordering. And we don't have a partial constructed instance that escapes.

But it's not critical concern here, it seems that volatile doesn't compact performance of this patch.

Thanks.





[CLJ-2069] lazy seq that encounters an exception has differing behavior on repeated use Created: 27/Nov/16  Updated: 29/Nov/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8, Release 1.9
Fix Version/s: None

Type: Defect Priority: Major
Reporter: TianJun Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: lazy
Environment:

OS X EI Capitan, Java HotSpot(TM) 64-Bit Server VM 1.8.0_101-b13


Attachments: Text File 0001-CLJ-2069-cache-exceptions-thrown-during-lazy-seq-rea.patch    
Patch: Code
Approval: Prescreened

 Description   

It seems the below does not compile with 1.8.0 and 1.9.0-alpha14, the same errors appear in both versions.

user=> (def fibonacci-1
  ((fn fib  [a b]
    (lazy-seq  (cons a  (fib b  (+ a b)))))
    0 1))

user=> (filter #(< % 100) fibonacci-1)

ArithmeticException integer overflow  clojure.lang.Numbers.throwIntOverflow (Numbers.java:1501)

user=> (filter #(< % 100) fibonacci-1)

NullPointerException   clojure.lang.Numbers.ops (Numbers.java:1013)

user=> (def fibonacci-2
         (lazy-cat [0 1] (map + (rest fibonacci-2) fibonacci-2)))

user=> (filter #(< % 100) fibonacci-2)

ArithmeticException integer overflow  clojure.lang.Numbers.throwIntOverflow (Numbers.java:1501)

user=> (filter #(< % 100) fibonacci-2)
(0 1 1 2 3 5 8 13 21 34 55 89)

Patch: 0001-CLJ-2069-cache-exceptions-thrown-during-lazy-seq-rea.patch

Proposal: Cache exceptions thrown during lazy-seq realization, to avoid re-running bodyfn which is declared as `^:once`

Prescreened by: Alex Miller



 Comments   
Comment by TianJun [ 27/Nov/16 10:42 AM ]

Maybe I should use take-while instead of filter.

However, can anyone explain why I get ArithmeticException while running

(filter #(< % 100) fibonacci-2)

for the first time and get the right result at the second time?

Comment by Nicola Mometto [ 28/Nov/16 3:27 AM ]

The NPE is caused by the interaction between:

  • lazy-seq throwing an exception while realizing part of the sequence
  • lazy-seq internally using ^:once for locals clearing

lazy-seq expects the bodyfn to be run exactly once and then the result to be cached, but if an exception gets thrown during the execution of bodyfn, the function will get run again when the sequence tries to be realized a second time. However if locals clearing has already happened (even partially) this means some locals in bodyfn will now be nil rather than holding their actual value.

Comment by Nicola Mometto [ 28/Nov/16 3:36 AM ]

Attached patch that fixes this issue





[CLJ-2067] (s/def ::a ::b) throws unable to resolve error if ::b is not defined Created: 22/Nov/16  Updated: 22/Nov/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Defect Priority: Minor
Reporter: Oliver George Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Vetted

 Description   

It should be possible to do `(s/def ::a ::b)` before declaring ::b.

Currently this throws an "Unable to resolve" error.

Alex indicated that everything should have delays but that they are missing in some cases. This seems like one of those cases.

Examples where things work fine are `(s/def ::a (s/and ::b))` and `(s/def ::a (s/keys :req [::b]))`.






[CLJ-2065] reduce-kv fails on subvec Created: 20/Nov/16  Updated: 21/Nov/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Approval: Triaged

 Description   

reduce-kv works as expected on vectors with the element index passed as the "key" argument. However, it fails with a subvec because clojure.lang.APersistentVector$SubVector does not implement IKVReduce.

(reduce-kv + 0 [1 2 3])
9

(reduce-kv + 0 (subvec [1 2 3] 1))
IllegalArgumentException No implementation of method: :kv-reduce of protocol: #'clojure.core.protocols/IKVReduce found for class: clojure.lang.APersistentVector$SubVector clojure.core/-cache-protocol-fn (core_deftype.clj:583)

One work around is to copy the subvec into a vector:

(reduce-kv + 0 (into [] (subvec [1 2 3] 1)))
6

Note however, the `vec` would not work here. Since Clojure 1.7, vec will return a subvec rather than copying.

(reduce-kv + 0 (vec (subvec [1 2 3] 1)))
IllegalArgumentException No implementation of method: :kv-reduce of protocol: #'clojure.core.protocols/IKVReduce found for class: clojure.lang.APersistentVector$SubVector clojure.core/-cache-protocol-fn (core_deftype.clj:583)



 Comments   
Comment by Steve Miner [ 20/Nov/16 12:53 PM ]

Here is my current work-around:

(extend-type clojure.lang.APersistentVector$SubVector
  clojure.core.protocols/IKVReduce
  (kv-reduce [subv f init]
    (transduce (map-indexed vector)
               (fn ([ret] ret) ([ret [k v]] (f ret k v)))
               init
               subv)))

In my tests it was usually faster to copy the subvec into a regular vector but I like the look of the transduce fix. It would probably be faster to add a native Java implementation in APersistentVector.java. I'm willing to do the work if the Clojure/core team wants a patch.





[CLJ-1860] 0.0 and -0.0 compare equal but have different hash values Created: 01/Dec/15  Updated: 17/Nov/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.9

Type: Defect Priority: Minor
Reporter: Patrick O'Brien Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: math

Attachments: Text File clj-1860-make-equals-false-for-pos-neg-0.0-v1.patch     Text File CLJ-1860-negative-zero-hash-eq-fix.patch    
Patch: Code and Test
Approval: Screened

 Description   

0.0 and -0.0 compare as equal but have different hash values:

user=> (= 0.0 -0.0)
true
user=> (hash -0.0)
-2147483648
user=> (hash 0.0)
0

This causes problems as the equality/hashing assumption is violated.

user=> #{[1 2 0.0] [1 2 -0.0]}
#{[1 2 -0.0] [1 2 0.0]}

user=> (hash-map 0.0 1 -0.0 2)
{0.0 2}

user=> (hash-map [0.0] 1 [-0.0] 2)
{[0.0] 1, [-0.0] 2}

user=> (array-map [0.0] 1 [-0.0] 2)
{[0.0] 2}

user=> (hash-set [0.0] [-0.0])
#{[0.0] [-0.0]}

Cause: The source of this is due to some differences in Java. Java primitive double 0.0 and -0.0 == but the boxed Double is NOT .equals(). See also: http://docs.oracle.com/javase/7/docs/api/java/lang/Double.html#equals%28java.lang.Object%29

Double equality is checked with == in Clojure, which will report true. Hashing falls through to .hashCode(), which returns different values (but is consistent with the .equals() result on the boxed form).

Approach: While there are times when 0.0 and -0.0 being different are useful (see background below), most Clojure users expect these to compare equal. IEEE 754 says that they should compare as equals as well. So the approach to take here is to leave them as equal but to modify the hash for -0.0 to be the same as 0.0 so that `=` and `hash` are consistent. The attached patch takes this approach.

Patch: CLJ-1860-negative-zero-hash-eq-fix.patch

Screened: Alex Miller

Alternative: Make 0.0 != -0.0. This approach affects a much larger set of code as comparison operators etc may be affected. The patch clj-1860-make-equals-false-for-pos-neg-0.0-v1.patch may be one way to implement this approach, and seems fairly small in the quantity of code affected (2 methods).

Background: https://en.wikipedia.org/wiki/Signed_zero



 Comments   
Comment by Stephen Hopper [ 09/Feb/16 10:45 PM ]

Just to summarize, it seems like this functionality in Clojure is the same as it is in Java:

0.0 == -0.0: true
new Double(0.0).hashCode(): 0
new Double(-0.0).hashCode(): -2147483648
new Double(-0.0).equals(new Double(0.0)): false

I can see pros and cons to both of the aforementioned approaches, as well as just leaving this one be. Does anyone else have any input on this one? Is this issue something we should rectify, or by changing it will we end up creating more problems than we solved?

Comment by Andy Fingerhut [ 10/Feb/16 12:38 AM ]

The Java behavior you demonstrate shows that in this case, they are not equals, so there is no need for the hashCode() values to be the same in order to satisfy the hash consistency property of equals and hashCode.

Clojure currently violates the hash consistency property that should ideally hold between clojure.core/= and clojure.core/hash, for 0.0 and -0.0.

Changing clojure.core/= so it is false would restore the hash consistency property for these values. Keeping (clojure.core/== 0.0 -0.0) true is hopefully something that will be maintained across any change, but that does not violate hash consistency, because that property has nothing to say about the value of clojure.core/==

Comment by Stephen Hopper [ 10/Feb/16 7:10 AM ]

Thanks for the explanation, Andy. That makes sense to me. I'll put together some tests and a possible solution for evaluation.

Comment by Stephen Hopper [ 10/Feb/16 11:20 AM ]

After diving into the source code a bit more, my preference is to modify the hash calculation for -0.0 to be the same as the hash calculation for 0.0. This will restore the hash consistency property without breaking other mathematical operations. Basically, if we update clojure.core/= to return false for (= 0.0 -0.0), we will need to update other functions (clojure.core/<, clojure.core/>, etc.) so that -0.0 and 0.0 still follow basic numerical properties surrounding equality and ordering. I'll add some tests and a possible patch to hash calculation for numbers for consideration.

Comment by Andy Fingerhut [ 10/Feb/16 12:40 PM ]

Someone, perhaps Patrick O'Brien , brought up a preference that making (= 0.0 -0.0) false would help in some applications, e.g. numerical applications involving normal vectors where it was beneficial if (= 0.0 -0.0) was false. I have no knowledge whether this preference will determine what change will be made to Clojure, if any. If someone finds a link to the email discussion that was in one of the Clojure or Clojure Dev Google groups, that would be a useful reference.

Comment by Stephen Hopper [ 11/Feb/16 9:47 PM ]

I've added a pair of patches for review on this one (one contains test updates and the other contains my proposed updates to the hash calculation). I realize that this is different than the approach proposed earlier in the ticket, but I think it should be the preferred approach. As I mentioned earlier, merely changing clojure.core/= to return false for (= 0.0 -0.0) would require also updating other numeric equality functions (clojure.core/<, clojure.core/>, etc.). More importantly, I feel that this behavior would be different than what most Clojure developers would expect.

For this reason, the patches I've updated merely modify the calculation of hashes with Numbers.java for Floats and Doubles for which isZero returns true to return the hashCode for positive 0.0 instead of negative 0.0.

Is this acceptable?

EDIT:
With these patches applied, we get the following behavior in the REPL:

user=> (= 0.0 -0.0)
true
user=> (hash 0.0)
0
user=> (hash -0.0)     
0
user=> (hash (float 0.0))
0
user=> (hash (float -0.0))
0
Comment by Andy Fingerhut [ 12/Feb/16 3:48 AM ]

Stephen, please see here http://dev.clojure.org/display/community/Developing+Patches for the commands used to create patches in the desired format.

Only a screener or Rich can say whether the patch is acceptable in the ways that matter for committing into Clojure.

Comment by Stephen Hopper [ 12/Feb/16 7:42 AM ]

I've corrected the format on my patch files and resubmitted them as one patch. Let me know if you see any issues with this. Also, it's worth pointing out that the first two patch files can be ignored / deleted.

Comment by Andy Fingerhut [ 14/Feb/16 1:30 PM ]

There are also instructions on that page for deleting old attachments, in the section titled "Removing patches", if you wished to do that.

A very minor comment, as the email address you use in your patches is completely up to you (as far as I know), but the one you have in your patch doesn't look like one that others could use to send you a message. If that was intentional on your part, no problem.

Comment by Stephen Hopper [ 14/Feb/16 1:48 PM ]

I've corrected the email address snafoo and removed the outdated patches. Thanks for walking me through this stuff, Andy.

Comment by Steve Miner [ 23/Feb/16 3:43 PM ]

The suggested fix is to make (hash -0.0) return the same as (hash 0.0). With the proposed patch, both will return 0. That happens to be the same as (hash 0). Not wrong at all, but maybe slightly less good than something else. Since you're making a change anyway, why not go the other way and use the -0.0 case as the common result?

I'm thinking that it would be a useful property if the hash of a long N is not equal to the hash of the corresponding (double N). In Clojure 1.8, zero is the only value I could quickly find where the long and the double equivalents have the same hash value.

The patch could be slightly tweaked to make (hash 0.0) and (hash -0.0)

return new Double(-0.0).hashCode()

The only change to the patch is to add the negative sign. The new hash result is -2147483648.

Admittedly this is an edge case, not a real performance issue. People probably don't mix longs and doubles in sets anyway. On the other hand, zeroes are kind of common. Since you're proposing a change, I thought it's worth considering a slight tweak.

Comment by Steve Miner [ 23/Feb/16 3:46 PM ]

Same for the float case, of course.

Comment by Alex Miller [ 24/Feb/16 1:36 PM ]

Looking at this agin, I'm ok with the approach and agree it's definitely a smaller change. Few additional changes needed in the patch and then I'll move it along:

  • Rather than `new Double(0.0).hashCode()` and `new Float(0.0).hashCode()`, move those values into `private static final int` constants and just return them.
  • In the comparison tests, throw -0.0M in there as well
  • Squash the patch into a single commit

Re Steve's suggestion, I do not think it's critical that long and double 0 hash differently and would prefer that double hashes match Java hashCode, so I would veto that suggestion.

Comment by Stephen Hopper [ 24/Feb/16 1:52 PM ]

Thank you, Alex. I'll make those updates.

Comment by Stephen Hopper [ 24/Feb/16 2:43 PM ]

Alex, I've attached the new patch file (CLJ-1860-negative-zero-hash-eq-fix.patch) to address the points from your previous post. Let me know if I missed anything.

Thanks!

Comment by Mike Anderson [ 25/Feb/16 7:44 PM ]

I may be late to the party since I have only just seen this, but I have a strong belief that 0.0 and -0.0 should be == but not =.

Reasons:

  • Anyone doing numerical comparison should use ==, so you want the result to be true
  • Anyone doing value comparison should use =, so you want the result to be false because these are different IEE784 double values. This includes set membership tests etc.

i.e. the Java code is doing it right, and we should be consistent with this.

Comment by Alex Miller [ 25/Feb/16 11:52 PM ]

I think there is consensus that == should be true, so we can set that aside and focus on =.

The reality is that there is no easy way to compare two collections with == (for example comparing [5.0 0.0 1.0] and [5.0 -0.0 1.0]). This is an actual use case that has been problematic for multiple people. While I grant there are use cases where 0.0 and -0.0 are usefully differentiable, I do not know of a real case in the community where this is the desired behavior, so I would rather err on the side of satisfying the intuition of the larger (and currently affected) population.

Also note that the Java code is doing it BOTH ways (primitive doubles are equal, boxed doubles are not), so I think that's a weak argument.

Comment by Alex Miller [ 26/Feb/16 8:44 AM ]

(after sleeping more on this...) It's possible that a better answer here is to expand what can be done with ==. I trust that when Rich looks at this ticket he will have his opinions which may or may not match up to mine and if so, we'll go in a different direction.

Comment by Andy Fingerhut [ 27/Feb/16 10:15 AM ]

Attachment clj-1860-make-equals-false-for-pos-neg-0.0-v1.patch dated Feb 27 2016 is a first cut at implementing a change where = returns false when comparing positive and negative 0.0, float or double.

As far as I can tell, there is no notion of positive and negative 0 for BigDecimal, so no change in behavior there.

Comment by Alex Miller [ 17/Nov/16 4:16 PM ]

Rich says: "0.0=-0.0, make hash the same"





[CLJ-2063] Show longest path explain error first Created: 17/Nov/16  Updated: 17/Nov/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0