<< Back to previous view

[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: 3
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-1496] Added a new arity to 'ex-info' that only accepts a message. Created: 08/Aug/14  Updated: 15/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





[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]))




Generated at Sun Feb 19 15:51:39 CST 2017 using JIRA 4.4#649-r158309.