<< Back to previous view

[UNIFY-8] Handle sets correctly Created: 21/May/14  Updated: 21/May/14

Status: Open
Project: core.unify
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Pascal Germroth Assignee: Fogus
Resolution: Unresolved Votes: 0
Labels: None
Environment:

unify 0.5.5



 Description   

Currently unify allows using sets as expressions and just uses them as sequences, which, depending on the order of items, causes the unification to fail or succeed:

(unify #{ '[aa a] '[bb b] } ; =seq=> [bb b] [aa a]
       #{ '[?a a] '[?b b] } ; =seq=> [?b b] [?a a]
) ; => {?a aa, ?b bb}
(unify #{  '[a a]  '[b b] } ; =seq=> [a a] [b b]
       #{ '[?a a] '[?b b] } ; =seq=> [?b b] [?a a]
) ; => nil, expected {?a a, ?b b}

Unify should either handle sets (not sure if the algorithm allows for that easily) or throw an IllegalArgumentException when passed a set, but not silently seq it and behave unpredictably like that.






[UNIFY-7] Dead code in occurs? function Created: 28/Jul/13  Updated: 28/Jul/13

Status: Open
Project: core.unify
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Greg Chapman Assignee: Fogus
Resolution: Unresolved Votes: 0
Labels: None


 Description   

In the cond in the occurs? function, this condition:

(zip/end? z) false

appears twice. Unless I very much misunderstand, the second clause will never succeed.

Also, I'm wondering if it makes sense (in core.unify) for composite? to be true for strings. It looks like this means occurs? will futilely check each character of a string looking for variables.






[UNIFY-6] Create tests for k&v map unification Created: 25/May/12  Updated: 25/May/12

Status: Open
Project: core.unify
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major
Reporter: Fogus Assignee: Fogus
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Substitution in map keys or values should both work. I doubt there are tests around this.






[UNIFY-5] core.unify locks Midje down to Clojure 1.3 Created: 26/Apr/12  Updated: 25/May/12  Resolved: 25/May/12

Status: Resolved
Project: core.unify
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major
Reporter: Brian Marick Assignee: Fogus
Resolution: Completed Votes: 0
Labels: None


 Description   

According to `lein2 deps :tree` core.unify 0.5.2 is the (a?) reason that Midje can't use Clojure 1.4.



 Comments   
Comment by Fogus [ 26/Apr/12 10:23 AM ]

I will put together a point release for the next version today or tomorrow (latest). Thank you.

Comment by Fogus [ 25/May/12 10:01 PM ]

Tomorrow came and went. I apologize. The 0.5.3 version has been deployed and should be available on Maven Central in the next few hours.





[UNIFY-4] Vectors of different length incorrectly unify Created: 08/Feb/12  Updated: 25/May/12  Resolved: 25/May/12

Status: Resolved
Project: core.unify
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Thomas Winant Assignee: Fogus
Resolution: Completed Votes: 0
Labels: unify


 Description   
(unify '[1 ?x] '[1])
=> {?x nil}

I expect this unification to fail.



 Comments   
Comment by Fogus [ 26/Apr/12 11:48 AM ]

Which version are you using?

Comment by Thomas Winant [ 26/Apr/12 1:43 PM ]

Version 5.2

Comment by Fogus [ 25/May/12 10:00 PM ]

This is not a perfect solution, but it works as a stopgap as this will be handled in the next 0.1.0 version. For now the 0.5.3 version has been deployed and should be available on Maven Central in the next few hours.





[UNIFY-3] Enhance documentation Created: 03/Feb/12  Updated: 03/Feb/12

Status: Open
Project: core.unify
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major
Reporter: Fogus Assignee: Fogus
Resolution: Unresolved Votes: 0
Labels: docs


 Description   

The current unify docs are spartan and "just the facts". It would be useful to have a set of docs that:

  • explain unification
  • explain the library use cases
  • show a simple example use





[UNIFY-2] Remove reflection warnings Created: 05/Jan/12  Updated: 05/Jan/12  Resolved: 05/Jan/12

Status: Resolved
Project: core.unify
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Fogus Assignee: Fogus
Resolution: Completed Votes: 0
Labels: performance, reflection, unify

Approval: Ok

 Description   
Reflection warning, clojure/core/unify.clj:30 - reference to field getClass can't be resolved.
Reflection warning, clojure/core/unify.clj:30 - reference to field isArray can't be resolved.

These reflective calls occur frequently enough that they should be resolved.



 Comments   
Comment by Fogus [ 05/Jan/12 7:37 AM ]

Fixed in 6b6d1130bf857439d1863f6fc574a7a6541b84b8.





[UNIFY-1] Cut 0.5.2 release jar Created: 14/Dec/11  Updated: 09/Jan/12  Resolved: 09/Jan/12

Status: Resolved
Project: core.unify
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major
Reporter: Fogus Assignee: Fogus
Resolution: Completed Votes: 0
Labels: None


 Description   

There are a couple of bug fixes that should make it into a +0.0.1 release.






[TTRACE-8] Documentation describing the features of tools.trace Created: 04/Apr/14  Updated: 14/May/14  Resolved: 14/May/14

Status: Closed
Project: tools.trace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Anders Conbere Assignee: Luc Préfontaine
Resolution: Completed Votes: 0
Labels: None


 Description   

I've been working on debugging some clojure systems and a coworker suggested that one helpful tool might be tools.trace. But the current documentation does nothing to help me understand how it might help.

For example here is a selection from the README:

```
(trace (* 2 3)) ;; To trace a value

(trace tag (* 2 3)) ;; To trace a value and assign a trace tag
```

But I as a total newb, have no idea what a trace is, why that would be useful, or why I would want to assign it to a tag (or even what a tag is).

I could pop open lein, get it installed, and play with it for a bit, but it's not clear to me why I should.



 Comments   
Comment by Luc Préfontaine [ 14/May/14 8:21 AM ]

I revamped the readme a bit but left stuff of larger scope open to experiment by the developper
(trace-ns/trace-vars and so forth).
Tracing existed for decades in Lisp, a simple Google search would give you an idea of what to expect.

Comment by Luc Préfontaine [ 14/May/14 8:23 AM ]

Modified README, no code change





[TTRACE-7] clone-throwable on type java.lang.Throwable either wastes computation or is missing cond Created: 02/Dec/13  Updated: 15/May/14  Resolved: 14/Mar/14

Status: Closed
Project: tools.trace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Luc Préfontaine
Resolution: Completed Votes: 0
Labels: None


 Description   

The body of clone-throwable is currently:

(extend-type java.lang.Throwable
  ThrowableRecompose
  (clone-throwable [this stack-trace args] 
    (try
      (let [ctor (.getConstructor (class this) (into-array [java.lang.String]))
            arg (first args)]
        (string? arg)
        (doto (.newInstance ctor (into-array [arg])) (.setStackTrace stack-trace))
        :else (doto (.newInstance ctor (into-array [(str arg)])) (.setStackTrace stack-trace)))
      (catch Exception e# this))))

Note that the return value of (string? arg) disappears into nowhere, as is the :else, and both doto's are always executed.

It appears that perhaps there should be a cond wrapped around the body of the let, as there is in the clone-throwable implementation for type java.nio.charset.CoderMalfunctionError earlier in the file, or these unnecessary bits of code should be removed.



 Comments   
Comment by Luc Préfontaine [ 14/Mar/14 5:14 PM ]

Yep, missing cond. Fixed.

Comment by Luc Préfontaine [ 14/Mar/14 5:53 PM ]

Missing cond added

Comment by Luc Préfontaine [ 15/Mar/14 3:53 PM ]

Official release 0.7.8, bad readme in 0.7.7





[TTRACE-6] Remove unnecessary (?) (def ~name) from deftrace Created: 29/Nov/13  Updated: 15/May/14  Resolved: 14/Mar/14

Status: Closed
Project: tools.trace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Andy Fingerhut Assignee: Luc Préfontaine
Resolution: Completed Votes: 0
Labels: None


 Description   

I was working on enhancements to a lint tool and trying it out on contrib libraries to see if any of them have redefinitions of symbols. It found that every use of deftrace causes a redefinition. Looking at deftrace's definition, I cannot see any reason for the (def ~name) before the later (defn ~name ...) expression. The later expression is done unconditionally, unlike the way deftest or defmulti always def a var, then redefine it if it was unbound.

I could easily be missing something important here, but is there a reason for the (def ~name)?



 Comments   
Comment by Nicola Mometto [ 29/Nov/13 10:52 AM ]

Removing the def, (deftrace x [] #'x) wouldn't compile anymore.

What about replacing the def with a declare?

Comment by Luc Préfontaine [ 14/Mar/14 5:13 PM ]

Switched to declare. This satisfies the compiler.

Comment by Luc Préfontaine [ 14/Mar/14 5:53 PM ]

Replaced the def by declare

Comment by Luc Préfontaine [ 15/Mar/14 4:05 PM ]

Official release 0.7.8, bad readme in 0.7.7





[TTRACE-5] test_trace.clj unnecessarily calls (run-tests) at the end Created: 29/Nov/13  Updated: 15/May/14  Resolved: 14/Mar/14

Status: Closed
Project: tools.trace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Andy Fingerhut Assignee: Luc Préfontaine
Resolution: Completed Votes: 0
Labels: None

Attachments: File ttrace-5-v1.diff    
Patch: Code

 Description   

Both 'mvn test' and 'lein test' run all tests twice with that (run-tests) near the end of the file. With that line removed, both of those commands run the set of tests once, as expected.



 Comments   
Comment by Andy Fingerhut [ 29/Nov/13 9:13 AM ]

Patch ttrace-5-v1.diff removes the unnecessary call to (run-tests). All tests pass once rather than twice after applying it

Comment by Luc Préfontaine [ 15/Mar/14 4:05 PM ]

Fixed in official release 0.7.8, bad readme in 0.7.7





[TTRACE-4] trace-ns traces all vars in the ns, not just fns Created: 12/Sep/13  Updated: 15/May/14  Resolved: 14/Mar/14

Status: Closed
Project: tools.trace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Chris Jeris Assignee: Luc Préfontaine
Resolution: Completed Votes: 0
Labels: None
Environment:

Clojure 1.5.1, tools.trace 0.7.6


Attachments: Text File tools.trace-4.patch    
Patch: Code and Test

 Description   

trace-ns traces every var in the given namespace, fns and non-fns alike. This is counterintuitive from the docstring (which says "trace all fns in the given name space") – when I have a data object like a map in a var, it may happen to implement IFn, but if I trace it I can't assoc onto it any more.

trace.demo> (def x {:foo 1 :bar 2})
#'trace.demo/x
trace.demo> (trace/trace-ns 'trace.demo)
nil
trace.demo> x
#<trace$trace_var_STAR_$fn__1682$tracing_wrapper__1683 clojure.tools.trace$trace_var_STAR_$fn__1682$tracing_wrapper__1683@2b0ce330>
trace.demo> (assoc x :baz 3)
ClassCastException clojure.tools.trace$trace_var_STAR_$fn__1682$tracing_wrapper__1683 cannot be cast to clojure.lang.Associative  clojure.lang.RT.assoc (RT.java:691)

I assume it's useful to retain the ability to trace IFn's that aren't fns at the level of trace-var. The attached patch instead changes trace-ns to check that traced values are fn? (not just ifn?) before wrapping them. Andy Fingerhut suggests that, if the ability to trace IFn's that aren't fns is not actually all that useful, it's simpler just to change ifn? to fn? in trace-var*.



 Comments   
Comment by Luc Préfontaine [ 14/Mar/14 5:19 PM ]

Agree. Fixed.

Comment by Luc Préfontaine [ 14/Mar/14 5:51 PM ]

Fix accepted. Only fns are traced.

Comment by Luc Préfontaine [ 15/Mar/14 3:52 PM ]

Official release 0.7.8, bad readme in 0.7.7





[TTRACE-3] untrace-var* removes only the highest level of tracing created by trace-var* Created: 29/Aug/13  Updated: 15/May/14  Resolved: 14/Mar/14

Status: Closed
Project: tools.trace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Luc Préfontaine Assignee: Luc Préfontaine
Resolution: Completed Votes: 0
Labels: enhancement
Environment:

Clojure 1.2-1.5.1



 Description   

Calling trace-var* (or using the macro trace-vars) multiple times on the same var(s) adds multiple tracing levels.
To remove tracing completely, untrace-vars* has to be called at least the same number of times on the traced var(s).

This behavior is annoying at the REPL where a single call to untrace-vars* would untracing much more simpler.
However such a behavior might be desirable when programatically driven.

Comments welcomed.



 Comments   
Comment by Luc Préfontaine [ 14/Mar/14 5:33 PM ]

After mulling over this, I think that the "recursive" behavior is not needed.
I see no added value even from a programmatic standpoint of allowing this behavior.

So calling trace-vars* on the same var has now on will not have any effect
after the first call.

Hence a single call to untrace-vars* will remove the trace at all times.

Took me some time but finally got through it

Comment by Luc Préfontaine [ 14/Mar/14 5:50 PM ]

trace-var* is now a noop if the fn is already traced

Comment by Luc Préfontaine [ 15/Mar/14 3:53 PM ]

Official release 0.7.8, bad readme in 0.7.7





[TTRACE-2] Eliminate a few uses of Reflection in tools.trace Created: 28/Oct/12  Updated: 15/May/14  Resolved: 01/Dec/12

Status: Closed
Project: tools.trace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Luc Préfontaine
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File ttrace-2-eliminate-reflection-v1.txt    
Patch: Code

 Description   

There are a few instances of reflection in tools.trace code that can be eliminated with suitable type hints.



 Comments   
Comment by Andy Fingerhut [ 28/Oct/12 10:32 PM ]

ttrace-2-eliminate-reflection-v1.txt dated Oct 28 2012 eliminates all current uses of reflection in tools.trace code by adding type hints.

Comment by Luc Préfontaine [ 14/Nov/12 6:14 PM ]

Done in 0.7.4

Comment by Luc Préfontaine [ 01/Dec/12 3:36 PM ]

Issued 0.7.5, mising readme but was fixed in 0.7.4

Comment by Luc Préfontaine [ 01/Dec/12 3:37 PM ]

Fixed in 0.7.5





[TTRACE-1] Two convenience predicate functions useful for IDE-tools Created: 23/Apr/12  Updated: 15/May/14  Resolved: 01/Dec/12

Status: Closed
Project: tools.trace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Frank Siebenlist Assignee: Luc Préfontaine
Resolution: Completed Votes: 0
Labels: enhancement

Patch: Code and Test

 Description   

In our IDE-tool clj-ns-browser, we show the user a list of vars and make it easy to add a trace by selecting a var and clicking a button.

In order to only enable the button for a var that is traceable, we use predicate "var-traceable?".

In order to change the button text from "trace" to "untrace", we use predicate "var-traced?".

Both "var-traceable?" and "var-traced?" are aware of tools.trace internals - ideally, we would like to remain oblivious to trace's inners...

Our implementations for the predicates is at: https://gist.github.com/2472261

Thanks for the great tool!



 Comments   
Comment by Luc Préfontaine [ 14/Nov/12 5:56 PM ]

Done, however the names where changed to traced? and traceable?.

Comment by Luc Préfontaine [ 14/Nov/12 6:18 PM ]

Done is 0.7.4

Comment by Luc Préfontaine [ 01/Dec/12 3:38 PM ]

Done, "good" version is 0.7.5 (readme & comments need an update in 0.7.4")





[TRDR-16] Reading Anonymous fns without arguments throws NPE Created: 20/Jun/14  Updated: 20/Jun/14  Resolved: 20/Jun/14

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Jozef Wagner Assignee: Nicola Mometto
Resolution: Declined Votes: 0
Labels: None


 Description   

This throws:

(clojure.tools.reader/read-string "#()")


 Comments   
Comment by Jozef Wagner [ 20/Jun/14 3:31 PM ]

It works when I used most recent master from repo. My bad, you can close this ticket. Sorry.





[TRDR-15] tools.reader 0.8.4 causes clojurescript to stop working in mysterious ways Created: 03/Jun/14  Updated: 04/Jun/14  Resolved: 04/Jun/14

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: James Cash Assignee: Nicola Mometto
Resolution: Declined Votes: 0
Labels: bug, cljs
Environment:

OS X 10.9.3, Java 1.8, Clojure 1.6.0



 Description   

If tools.reader 0.8.4 is included in a project's dependencies, then clojurescript fails to work in a browser, with the error that cljs.core.PersistentArrayMap is undefined. I have created a sample project demonstrating this at https://github.com/jamesnvc/cljs-toolreader-debugging.

I also apologize that I'm not sure if this is a problem in tools.reader or clojurescript. If there is a better place for me to report this issue, please let me know!



 Comments   
Comment by Nicola Mometto [ 04/Jun/14 3:33 AM ]

This is a bootstrap issue in clojurescript.
tools.reader 0.8.4 introduced :file metadata, clojurescript needs to dissoc it here https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/analyzer.clj#L1490 to work with this version.

I suggest you open a ticket in che clojurescript jira making that change if you want to make clojurescript work with 0.8.4

Comment by James Cash [ 04/Jun/14 9:04 AM ]

Thank you very much Nicola, I opened the issue on the clojurescript JIRA





[TRDR-14] Feature Expressions Created: 15/May/14  Updated: 05/Aug/14

Status: Open
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Ghadi Shayban Assignee: Nicola Mometto
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File tools.reader-feature-expressions-2.diff     File tools.reader-feature-expressions-3.diff     File tools.reader-feature-expressions.diff    
Patch: Code and Test

 Description   

Feature expressions based directly on Common Lisp.

Implementation based on Roman's patch for CLJS-27. #+ #- and or not are supported. Unreadable tagged literals are suppressed through *suppress-read*.

For example, reading the following with features set to #{:clj} returns :baz

#+bad/platform #foo bar :baz

Initial feature names for each platform will be clj, cljs, and cljr.

CLJ-1424 is the accompanying patch for Clojure.

Patch: tools.reader-feature-expressions-3.diff



 Comments   
Comment by Nicola Mometto [ 19/May/14 6:56 PM ]

I looked over the patch and it mostly looks good, a few comments:

  • does *suppress-read* need to be exposed?
  • can you add a docstring for *features*?
  • should *suppress-read* affect read-eval?

That said, has it been decided that this is how feature expressions will be implemented in 1.7?
If not, I'm going to wait to merge this in until a decision is taken, this is a major design point and I don't want to diverge from the official clojure take on this even for a single release: people might start using it, especially since t.r is now the default reader for cljs.

Comment by Ghadi Shayban [ 19/May/14 7:23 PM ]

Yes thank you for not merging. This is WIP and just one approach for feature expressions. There seem to be at least two couple diverging approaches emerging from the various discussion (Brandon Bloom's idea of read-time splicing being the other.) I would definitely not merge until the community and Rich weighs in.

In any case having all Clojure platforms be ready for the change is probably essential. Also backwards compatibility of feature expr code to Clojure 1.6 and below is also not trivial.

1) *suppress-read* probably doesn't need to be exposed
2) *suppress-read* should probably imply *read-eval* false. You wouldn't want read-ctor to load any classes or call any code or constructors

Comment by Alex Miller [ 04/Aug/14 12:56 PM ]

Updated patch to reflect clj and cljs as initial feature names.





[TRDR-13] Better documentation wrt interaction between reader-types and java readers Created: 11/Apr/14  Updated: 11/Apr/14

Status: Open
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major
Reporter: Nicola Mometto Assignee: Nicola Mometto
Resolution: Unresolved Votes: 0
Labels: None





[TRDR-12] Misplaced doc string for function newline? Created: 23/Dec/13  Updated: 23/Dec/13  Resolved: 23/Dec/13

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Andy Fingerhut Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None

Attachments: File trdr-12-v1.diff    

 Description   

Found using Eastwood



 Comments   
Comment by Nicola Mometto [ 23/Dec/13 7:06 PM ]

Fixed: https://github.com/clojure/tools.reader/commit/1c9633400fdfdf6852d355edd9c44a721b0938ae





[TRDR-11] Stack overflow on whitespace in reader/read and edn/read Created: 05/Dec/13  Updated: 05/Dec/13  Resolved: 05/Dec/13

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Paul Michael Bauer Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File fix_stack_overflow_in_reader.patch    
Patch: Code

 Description   

reader/read and edn/read call themselves recursively for several encountered character classes - whitespace, comments.
This is high-risk for stack overflow, especially for machine-generated data.

Motivating Use Case: cljs files generated via cljx are particularly prone to trigger tools.reader stack overflow errors. Cljx replaces un-included forms with homomorphic whitespace blocks to preserve line number and column errors on compilation.



 Comments   
Comment by Nicola Mometto [ 05/Dec/13 3:50 PM ]

Fixed: https://github.com/clojure/tools.reader/commit/01b53cb61b586e78cf3f70f12ba2adbbdb7abb25

Thanks, I took the liberty of changing the commit description.

Comment by Paul Michael Bauer [ 05/Dec/13 3:53 PM ]

Thanks for changing the commit message. facepalm
Missed that when I generated the patch





[TRDR-10] Add end-line and end-column metadata to read objects Created: 25/Oct/13  Updated: 08/Nov/13  Resolved: 06/Nov/13

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Alexander Redington Assignee: Alexander Redington
Resolution: Completed Votes: 0
Labels: None

Attachments: File TRDR-10-line-numbering.diff    
Patch: Code and Test

 Description   

Per TRDR-9, add end-line and end-column metadata information to every form read, when the underlying reader implements the IndexingReader protocol.



 Comments   
Comment by Alexander Redington [ 25/Oct/13 12:03 PM ]

Patch to include end-line and end-column numbers on all forms read, with tests.

Comment by Nicola Mometto [ 06/Nov/13 3:26 PM ]

Applied to master: https://github.com/clojure/tools.reader/commit/a7251e87c5530e04c3c7c4a983f30030f38730df

I'm sorry I didn't apply this earlier but JIRA didn't send me an email notifying me that you updated the patch even though I am watching the issue.

Comment by Alexander Redington [ 08/Nov/13 7:31 AM ]

I've only been working on this in Friday time, and even that has been sporadic, so nothing to worry about!

Working with you on this has been excellent, thanks for accepting the suggestions and patches!





[TRDR-9] Add more source metadata to read objects Created: 27/Sep/13  Updated: 02/Sep/14  Resolved: 20/Nov/13

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Alexander Redington Assignee: Alexander Redington
Resolution: Completed Votes: 0
Labels: None

Attachments: File TRDR-9-source-logging.diff    
Patch: Code and Test

 Description   

I'm working on a static analyzer for Clojure, where I'd like to have a reader which provides precise form coordinates and source representations.

Specifically this enables analyses where it's straightforward to bind an object like:

(fn* [p1__3615#] (inc (conj p1__3615#)))

to the source form which it was read from:

"#(inc (conj %))"

Would this kind of functionality be considered appropriate to tools.reader? Specifically I'd like to make a change such that

  • Read data is unchanged
  • When an IMeta is read, additional information is embedded in the metadata, but no existing metadata is changed
  • The additional metadata would include the source character string, the ending column, and the ending line from the source stream

As an example, reading a stream that contained the following on the first line:

#(inc (conj %))

Would return a list as follows:

(fn* [p1__3615#] (inc (conj p1__3615#)))

And would change the metadata from:

{:line 1, :column 1}

to augment it with more metadata, as follows:

{:line 1, :column 1, :end-line 1, :end-column 15, :source "#(inc (conj %))"}

I'd happily contribute a patch to include this behavior if you'd be inclined to accept it. I'd like to first see if it would be considered appropriate, and also discuss any other concerns you might have (such as having this behavior be optionally enabled, etc)



 Comments   
Comment by Nicola Mometto [ 28/Sep/13 1:48 PM ]

I would definitely take a patch to add such a behaviour, with the condition that this should not degrade the performances too much (a 2x slowdown would be acceptable though).

I don't feel like this needs to be optional behaviour, we already provide column/line info by default, if it's not needed the user might just ignore it.

Comment by Alexander Redington [ 18/Oct/13 12:49 PM ]

Just like to touch base as I've been quiet for a little while, but haven't forgotten about this. It's been my primary goal on Friday time.

I currently have a naive implementation that works, but blows our performance goal by a 5x factor. I think this is solvable with a little more finesse in source logging.

It appears that end-line and end-column information can be obtained for almost no additional computational cost. (less than 2% additional processor time).

I've committed my WIP to: https://github.com/aredington/tools.reader/tree/metadata-logging

Also worth discussing at some point in time: some tests around syntax quoting and metadata reading start failing. For syntax quote, the forms returned are significantly more verbose, including metadata assertions with the source attribution. For metadata, the metadata values read now have an additional :source key included that specifies the source string the forms were read from. In both cases, the expectations of the tests are violated, but I'm not inclined to say that the end result is incorrect without discussion.

Comment by Nicola Mometto [ 22/Oct/13 11:57 AM ]

Awesome, I haven't had a chance to read through the implementation but I'll do so as soon as possible.
If I'm reading the last commit message correctly, it looks like since your comment here you were able to reduce performance loss to 2x, that's cool.

Can you please make the end-line/end-column modifications into another ticket? I can merge that as soon as you upload the patch.

Regarding the failing tests – looks like those are inevitable, it's definitely not a bug and those tests should be fixed to match the new behaviour (fixing those should be just a matter of manually attatching the :source metadata to the forms to be read by the clojure reader)

Comment by Nicola Mometto [ 24/Oct/13 11:39 AM ]

After some more thoughts, I think it should be better to have a "InfoPushBackReader" (not sure if it's the best name) wrapping an IndexingPushBackReader that provides source data, this way we only get the performance hit when we effectively need the :source info instead of making every Reader collect source data even when we don't want/need it.

Would this approach work for your use case?

Comment by Alexander Redington [ 25/Oct/13 8:04 AM ]

Absolutely. I'll make two patches and we will have two paths for metadata:

  • Default case; if line numbering information is available, end-line and end-column will be captured. The performance hit from this is marginal (2-3% slower), but it does provide sufficient information for, e.g. editors, IDEs to precisely indicate what portions of a source file originate what forms. This patch will be submitted in a new ticket.
  • Enhanced case; using a new InfoPushbackReader (SourceInfoPushbackReader? SourceLoggingPushbackReader?) we'll log precise source information on each form read. The performance hit from this will be substantial (100% slower), but it isolates the performance impacts to those users who deliberately want more information and are willing to pay the performance cost. This patch will be attached to this issue (TRDR-9)
Comment by Alexander Redington [ 25/Oct/13 8:27 AM ]

Created TRDR-10 for end-line and end-column info.

Comment by Nicola Mometto [ 06/Nov/13 4:04 PM ]

Looks like CLJS will also beenfit from this, see http://dev.clojure.org/jira/browse/CLJS-658

+1 for SourceLoggingPushbackReader

Comment by Alexander Redington [ 15/Nov/13 1:35 PM ]

On https://github.com/aredington/tools.reader I have it nearly working save that nested collections (e.g. [1 2 [3 4 5]]) fail to read correctly.

The top collection will read correctly with the following as the source string: "[1 2 [3 4 5]]", the nested vector reads as "3 4 5]".

I tried taking the macro dispatching char and prepending it to the source log at the appropriate source logging frame, and that resolves nested collections, but causes metadata to read erroneously, e.g. {:foo :baz} 'zot will read with source string "^{:foo :baz} 'zot".

Gonna keep digging to try and resolve this because source logging is very close, but just wanted to update on current status as I'd been quiet for a while.

Comment by Nicola Mometto [ 15/Nov/13 8:02 PM ]

I'll take a look at your current implementation and see if I can help you spot what's going wrong, thanks again for the effort you're putting into this.

Comment by Nicola Mometto [ 16/Nov/13 8:49 AM ]

I opened a pull request https://github.cam/aredington/tools.reader/pull/1 that should fix all the issues you were having, as well as fixing another minor bug.

If you find my edits reasonable please merge it and then squash the commits and I'll merge the patch upstream

Comment by Alexander Redington [ 20/Nov/13 1:23 PM ]

Squashed commit with working source logging

Comment by Nicola Mometto [ 20/Nov/13 3:17 PM ]

Fixed: https://github.com/clojure/tools.reader/commit/f9e55071d82a443db3fac1c2feda059d0215bb90

Thanks

Comment by Nikita Beloglazov [ 02/Sep/14 3:40 PM ]

As I see current tools.reader still not able to return position info (line, column numbers) for strings, numbers, booleans and other primitives that don't implement IMeta interface. Is there plans to fix it somehow? I was thinking how it can be fixed and only idea I came up with was to read input data into intermediate datastructure like:

{:type :vector
:children [{:type :string
:value "hello"
:line 1
:column 2}]
:line 1
:column 4}

And then convert it to clojure datastructures: ["hello"] so changes are invisible from outside. Additionally you can pass additional option like :raw-format true and it will return those internal datastructure.

Would it be the change that will be accepted or is it too drastic? I can start working it. I think this functionality is really needed for libraries linter-like libraris that checks whether code is formatted correctly and need to know position of each lexem.

Comment by Nicola Mometto [ 02/Sep/14 4:21 PM ]

I thought about this issue a while ago and I'd be definitely more than happy to take a patch for this.
What I'd like to see is a tools.reader.parser namespace with a parse function that returns a parse tree, and then tools.reader be just a compiler from that parse tree back to clojure, however I'm worried about the performance implications of that; I've deliberately avoided writting tools.reader with multimethods in favour of direct dispatch via `case` for performance reasons, this would probably involve an unacceptable performance hit.

So probably keeping the parser and the reader separated, even at the cost of having to copy&paste most of the current implementation is the way to go; obviously if you want to try to unify the two feel free to do so.

Can you open a new enhancement ticket for this?





[TRDR-8] Fix for parsing of tagged literals Created: 12/Sep/13  Updated: 13/Sep/13  Resolved: 13/Sep/13

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Alex Coventry Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: bug, patch

Attachments: Text File 0001-Fix-for-ctor-reading.patch     Text File reader.patch    
Patch: Code
Approval: Ok

 Description   

Parsing of tagged literals fails at the moment, e.g.

user> (require '[clojure.tools.reader :as r] :reload) (r/read-string "#java.lang.String[\"Hi\"]")
nil
ArrayIndexOutOfBoundsException 15 clojure.lang.RT.aget (RT.java:2239)

Attached patch corrects this problem, and brings the clojure logic in line with what's in LispReader.java



 Comments   
Comment by Nicola Mometto [ 13/Sep/13 7:18 AM ]

Thanks for the patch, can you post a patch that is the result of git format-patch so that we can keep your authorship on the commit?
(apply the patch, git add <path/to/reader.clj>, git commit -m "Fix ctor reading", git format-patch origin/master)

Also, can you please put the result of (count entries) in the let so that we compute it only once?

Thanks

Comment by Alex Coventry [ 13/Sep/13 12:06 PM ]

No worries, Nicola. I've attached a revised patch.

Best regards,
Alex

Comment by Nicola Mometto [ 13/Sep/13 12:38 PM ]

fixed https://github.com/clojure/tools.reader/commit/d9374f90448a4ff52ad83a2b75be2fa520a24db8





[TRDR-7] Anonymous variadic fn arg not read properly Created: 12/Aug/13  Updated: 14/Aug/13  Resolved: 14/Aug/13

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Chas Emerick Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None

Patch: Code and Test

 Description   

Compare clojure.core/read-string:

user=> (read-string "#(last %&)")
(fn* [& rest__461#] (last rest__461#))

vs. clojure.tools.reader/read-string:

user=> (reader/read-string "#(last %&)")
(fn* nil (last rest__458#))

The most immediate manifestation of this is that anonymous function forms in ClojureScript >= 0.0-1853 will not compile. I'm filing an issue to track this there as well.



 Comments   
Comment by Nicola Mometto [ 14/Aug/13 9:44 AM ]

Sorry for the delay, fixed with https://github.com/clojure/tools.reader/commit/8f48f83a5ad0dbd7f4c8e612c4ab764d418feb93

I'm releasing 0.7.6 ASAP





[TRDR-6] Some uses of reflection in tools.reader code slow it down unnecessarily Created: 13/Apr/13  Updated: 13/Apr/13  Resolved: 13/Apr/13

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File trdr-6-eliminate-reflection-with-type-hints-patch-v1.txt    
Patch: Code

 Description   

Attached patch uses type hints to eliminate several instances of reflection in the tools.reader code.

FYI, you can run 'lein check' to cause Leiningen to compile the code with warn-on-reflection true, I believe for every source file (not sure about the tests, but reflection isn't such a big deal in them anyway).



 Comments   
Comment by Nicola Mometto [ 13/Apr/13 12:13 PM ]

Andy, I don't see any patch attached to this ticket, I think you forgot to attach it.

(P.S. thanks, I didn't know about 'lein check')

Comment by Andy Fingerhut [ 13/Apr/13 12:19 PM ]

Patch trdr-6-eliminate-reflection-with-type-hints-patch-v1.txt dated Apr 13 2013 eliminates all occurrences of reflection found in latest version of tools.reader. Please check them carefully before committing them, especially the ones in default_data_readers.clj.

And I know the reflection warnings in default_data_readers.clj exist in Clojure's code, too, where you copied those from. CLJ-1080 has a patch addressing those and many other reflections within Clojure's code.

Comment by Nicola Mometto [ 13/Apr/13 12:59 PM ]

Andy, casting to char makes tools.reader crash under clojure-1.3, apparently casting to char is possible only from clojure 1.4.

Could you please submit a patch removing reflection in default_data_readers.clj and the docstring fixes only while I try to figure out a way to maintain clojure 1.3 compatibility and remove the reflection? (or if you have an idea on how to do it, you're more than welcome )

EDIT:
I edited your patch manually, and pushed a commit to type hint to char only for >clojure-1.3.0
I don't know if there is a way to avoid the reflection from clojure 1.3.0, but for the all the other versions of clojure all reflection is gone, thanks!

Comment by Andy Fingerhut [ 13/Apr/13 2:43 PM ]

Great. I was off-line there for a while, but glad you noticed the 1.3 compatibility issue where I didn't, and glad you found a different way to eliminate reflection with 1.4 and later.





[TRDR-5] Additional "Differences from LispReader.java" for README Created: 08/Apr/13  Updated: 10/Apr/13  Resolved: 10/Apr/13

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None


 Description   

Suggested addition to "Differences from LispReader.java" section of README.md:

read is capable of reading the symbol / with an explicit namespace, e.g. foo//, whereas clojure.lang.LispReader/read throws an exception. Refer to CLJ-873. Except for this special case, read throws an exception if a symbol contains more than one / character, whereas clojure.lang.LispReader/read allows them, returning a symbol with one or more / characters in its namespace name.



 Comments   
Comment by Nicola Mometto [ 10/Apr/13 5:20 AM ]

Thanks, fixed https://github.com/clojure/tools.reader/commit/f689cb283d1fb539a6cabbefd4036f620dabe693





[TRDR-4] Typo: Change 'end' to 'edn' in 'clojure.tools.reader.end/read-string' in README Created: 15/Mar/13  Updated: 15/Mar/13  Resolved: 15/Mar/13

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Andy Fingerhut Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None


 Description   

Just a miniscule typo



 Comments   
Comment by Nicola Mometto [ 15/Mar/13 9:50 AM ]

Fixed, thanks





[TRDR-3] Make read-line available in clojure.tools.reader and/or clojure.tools.reader.edn namespace? Created: 15/Mar/13  Updated: 15/Mar/13  Resolved: 15/Mar/13

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None


 Description   

You mention the enhanced version of read-line that takes a reader argument in the README, but it seems like it is only available in the clojure.tools.reader.reader-types namespace. Is that intentional? Perhaps making it available in the clojure.tools.reader and/or clojure.tools.reader.edn namespace might be more convenient for the user of the library? I understand that the current implementation of read-line is Java-specific, so maybe this is the only reasonable way to expose it.

In any case, documenting the namespace in which this enhanced read-line is available in the README would be good.



 Comments   
Comment by Nicola Mometto [ 15/Mar/13 10:26 AM ]

I added more documentation on the readme, making it clear in which namespace read-line is defined.
(https://github.com/clojure/tools.reader#public-api, https://github.com/clojure/tools.reader#differences-from-lispreaderjava)

read-line definitely belongs in the c.t.r.reader-types namespace, since it works on reader-types and returns a string, so it's not a reader function.

I hope this addresses this ticket, I'm closing it, feel free to reopen it you think the doc needs to be more clear.

Comment by Andy Fingerhut [ 15/Mar/13 7:04 PM ]

Looks thoroughly documented to me – above and beyond what I would have asked for. Thanks.





[TRDR-2] Fix a few README typos Created: 14/Feb/13  Updated: 01/Aug/13  Resolved: 15/Feb/13

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File trdr-2-fix-readme-typos-patch-v1.txt    
Patch: Code

 Description   

There are a few typos in the README



 Comments   
Comment by Andy Fingerhut [ 14/Feb/13 3:16 PM ]

Nothing major here. Just a few typo fixes for the README that I found.

Comment by Nicola Mometto [ 15/Feb/13 4:19 AM ]

Thanks, applied





[TRDR-1] read-char returns nil for some input types, on first attempt to read a char Created: 12/Feb/13  Updated: 01/Aug/13  Resolved: 12/Feb/13

Status: Closed
Project: tools.reader
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Andy Fingerhut Assignee: Andy Fingerhut
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File trdr-1-correct-read-char-v1.txt    
Patch: Code

 Description   

The return value from Java's .read method is -1 for EOF. A couple of condition checks in the code appear to be reversed. See the patch.



 Comments   
Comment by Nicola Mometto [ 12/Feb/13 4:47 PM ]

Thanks, tests for reader-types are now on the TODO list





[TNS-25] A few doc string typos Created: 09/Sep/14  Updated: 19/Sep/14  Resolved: 19/Sep/14

Status: Resolved
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Andy Fingerhut Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File tns-25-v1.patch    
Patch: Code

 Description   

Attached patch corrects a few misspellings in tools.namespace doc strings



 Comments   
Comment by Stuart Sierra [ 19/Sep/14 9:06 AM ]

Applied in https://github.com/clojure/tools.namespace/commit/09e1e1fd81faed4e24399bf68bf78e87b32eb68b

Thanks.





[TNS-24] Broken tracker after mis-named namespace Created: 07/Sep/14  Updated: 11/Sep/14

Status: Open
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 0
Labels: None


 Description   

A tools.namespace dependency tracker can get into an inconsistent state after attempting to load a file with a namespace declaration that does not match the file's name.

Steps to reproduce:

In a new Clojure project, create a file foo.clj on the classpath containing the namespace declaration (ns wrong-ns-declaration-for-foo)

Then at the REPL:

user=> (use 'clojure.tools.namespace.repl)
nil
user=> (refresh)
:reloading (wrong-ns-declaration-for-foo)
:error-while-loading wrong-ns-declaration-for-foo
#<FileNotFoundException java.io.FileNotFoundException: Could not locate wrong_ns_declaration_for_foo__init.class or wrong_ns_declaration_for_foo.clj on classpath: >

Edit the file foo.clj so that its namespace declaration is correct: (ns foo)

But at the REPL, refresh still doesn't work:

user=> (refresh)
:reloading (foo wrong-ns-declaration-for-foo)
:error-while-loading wrong-ns-declaration-for-foo
#<FileNotFoundException java.io.FileNotFoundException: Could not locate wrong_ns_declaration_for_foo__init.class or wrong_ns_declaration_for_foo.clj on classpath: >

Since tools.namespace 0.2.5, the workaround is to call clear:

user=> (clear)
{}
user=> (refresh)
:reloading (foo)
:ok


 Comments   
Comment by Andy Fingerhut [ 11/Sep/14 6:38 PM ]

Such files introduce problems for all kinds of tools, not just tools.namespace. I don't know all of the consequences, but two are listed here: https://github.com/jonase/eastwood#check-consistency-of-namespace-and-file-names

I have added warnings in Eastwood whenever it finds files like this, and doesn't do any linting checks on any files until such things are corrected.





[TNS-23] Refresh breaks when changing priority of conflicting methods in `prefer-method` Created: 30/Aug/14  Updated: 07/Sep/14  Resolved: 04/Sep/14

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Jeff Terrell Assignee: Stuart Sierra
Resolution: Declined Votes: 0
Labels: bug
Environment:

Clojure 1.6, CTN 0.2.5.



 Description   

Try defining a record that implements the clojure.lang.IDeref protocol. Create an instance, then try to print it, and you'll get an exception like so:

java.lang.IllegalArgumentException: Multiple methods in multimethod 'print-method' match dispatch value: class <record-class-name> -> interface clojure.lang.IPersistentMap and interface clojure.lang.IDeref, and neither is preferred

The solution to this is to include this line after the defrecord form:

(prefer-method print-method clojure.lang.IDeref clojure.lang.IPersistentMap)

This is all fine so far. The problem is that if you change the preference and call (ctn/refresh), you get an exception:

Error refreshing environment: java.lang.IllegalStateException: Preference conflict in multimethod 'print-method': interface clojure.lang.IPersistentMap is already preferred to interface clojure.lang.IDeref, compiling<source-file.clj>:164:22)



 Comments   
Comment by Stuart Sierra [ 04/Sep/14 4:18 PM ]

This is because prefer-method is a global side-effect at the top-level of a namespace.

The only way to work around this would be to call remove-method before reloading the namespace. Knowing to do that would require deep analysis far outside the scope of tools.namespace, but it can be done in application-specific code in a wrapper around refresh.

Comment by Jeff Terrell [ 05/Sep/14 2:05 PM ]

Fair enough. Thanks for looking into it.

Is this limitation worth documenting somewhere? Perhaps either in the API docs or in the c.t.n README?

Comment by Stuart Sierra [ 05/Sep/14 2:32 PM ]

Will add a note in README

Comment by Stuart Sierra [ 07/Sep/14 10:37 AM ]

Added warning to README in [commit 57e5658c8d9154711979019d85b279ad5f6898c9](https://github.com/clojure/tools.namespace/commit/57e5658c8d9154711979019d85b279ad5f6898c9).





[TNS-22] Function ns-file-name makes incorrect call to clojure.string/replace when File/separator is "\\" (e.g. Windows) Created: 24/Aug/14  Updated: 07/Sep/14  Resolved: 07/Sep/14

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File tns-22-v1.patch    
Patch: Code

 Description   

On Windows, where File/separator is a string consisting of a single backslash character:

user=> (require '[clojure.string :as str])
nil

;; This works as expected, because str/replace with second arg being a string handles arbitrary strings as the third arg
user=> (str/replace "foo.bar" "." File/separator)
"foo\\bar"

;; This gives an exception, because str/replace with second arg being a regex treats backslash characters in third arg specially.
user=> (str/replace "foo.bar" #"\." File/separator)
StringIndexOutOfBoundsException String index out of range: 1  java.lang.String.charAt (String.java:658)

;; This works as expected, but is more complex than the first alternative above
user=> (str/replace "foo.bar" #"\." (str/re-quote-replacement File/separator))
"foo\\bar"

I'd recommend using str/replace calls like the first one above when the things you want to replace are constant strings.



 Comments   
Comment by Andy Fingerhut [ 24/Aug/14 7:01 PM ]

Patch tns-22-v1.patch changes the only 2 calls to clojure.string/replace I found in tools.namespace that could have their second arg be a constant string rather than a regex.

Comment by Stuart Sierra [ 07/Sep/14 11:32 AM ]

Thanks.

Patch applied in https://github.com/clojure/tools.namespace/commit/a44f709e9e0a35a0eac6ec8eb6a2845c54efb97f

Comment by Stuart Sierra [ 07/Sep/14 2:39 PM ]

In release 0.2.6





[TNS-21] tools.namespace ignores ns form dependencies inside vectors Created: 23/Aug/14  Updated: 04/Sep/14  Resolved: 04/Sep/14

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Stuart Sierra
Resolution: Declined Votes: 0
Labels: None


 Description   

Cross reference: TLOG-13

For example, tools.logging has this ns form (metadata omitted):

(ns clojure.tools.logging
  [:use
   [clojure.string :only [trim-newline]]
   [clojure.pprint :only [code-dispatch pprint with-pprint-dispatch]]]
  [:require [clojure.tools.logging.impl :as impl]])

tools.namespace ignores these dependencies, only processing those that are in list subforms of the ns form.

Not sure if the best way to handle this is to update tools.logging or tools.namespace. tools.logging seems pretty unusual in using vectors like this, but the Clojure compiler seems to accept it just fine.



 Comments   
Comment by Stuart Sierra [ 24/Aug/14 1:29 PM ]

The ns macro docstring is quite clear that references inside it should be lists. I've seen a lot of weird ns forms, and this is the first time I've seen vectors used this way.

I would say I'm surprised that vectors work here, except nothing weird about the ns macro surprises me any more.





[TNS-20] tracker's unload order sometimes incorrect Created: 23/Aug/14  Updated: 20/Sep/14

Status: Open
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Andy Fingerhut Assignee: Stuart Sierra
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File tns-20-v1.patch     Text File tns-20-v2.patch    

 Description   

The attached patch contains a new test namespace clojure.tools.namespace.load-unload-order-test that demonstrates the incorrect unload order, if you leave out the proposed fix in file track.clj



 Comments   
Comment by Andy Fingerhut [ 23/Aug/14 3:02 PM ]

Patch tns-20-v1.patch contains a test that demonstrates the bug, and contains a proposed fix. Please scrutinize the proposed fix carefully, as I haven't yet convinced myself 100% that it is the right fix.

Also adds one more dependency check in dependency_test.clj that I discovered while reviewing those tests, to model my dependency checking tests on.

Comment by Andy Fingerhut [ 24/Aug/14 4:52 PM ]

I've got some code that checks that the load and unload orders in a tools.namespace tracker are consistent with its dependencies. It is similar to the checks done now in dependencies_test.clj, except it assumes that the dependencies in the tracker are correct and verifies the load and unload orders against those.

If you would be interested in a patch that added these checks to tools.namespace, perhaps along with a debug/consistency-check flag that when true causes these checks to be run every time a tracker is updated, let me know.

Comment by Stuart Sierra [ 07/Sep/14 11:11 AM ]

Thanks for this, Andy. I will need to study it further to convince myself this is the correct behavior.

To help, can you describe what the observed problem would be from a user's point of view?

We cannot assume that the dependencies in the tracker are always correct. I have demonstrable cases where they are wrong. Usually that takes the form of nonexistent namespaces left in the dependency graph from files that were deleted or had an invalid ns declaration.

Comment by Andy Fingerhut [ 07/Sep/14 8:10 PM ]

I have not used tools.namespace for the reload workflow that it was originally developed for, so I can't say with certainty what the effect on a user would be, but I can make some guesses that you can probably confirm more quickly than I can.

The test case in the patch is one where a few Clojure source files are added to a tracker using function c.t.n.dir/scan-all. No other changes are made to the tracker. At that point, the dependencies are completely correct, and the load order calculated from those dependencies honors them, but the unload order does not.

If that tracker were then used in a call to c.t.n.reload/track-reload, track-reload would call remove-lib on a library B before calling remove-lib on a library A, even though A requires or uses B.

I guess your question confuses me a bit. Do you believe tools.namespace should only create trackers that have load and unload orders that honor the dependencies? Or is that a wrong assumption I was making from reading the implementation?

Comment by Andy Fingerhut [ 08/Sep/14 5:52 PM ]

Attaching slightly cleaned up patch tns-20-v2.patch. Identical to tns-20-v1.patch except it avoids copying a function into the new test namespace by adding a dependency on the test namespace where it is defined.

Also updates the name of a deftest I had copied but not renamed in v1 of the patch.

Comment by Stuart Sierra [ 19/Sep/14 4:19 PM ]

I think I'm starting to get a handle on this. There's a lot going on here, and it's been more than two years since I wrote most of this code, so bear with me.

At track.clj line 78 we compute the dependency order for unloading namespaces based on the dependency graph (the local deps) as it existed before the most recent set of changes. I believe that this is correct, or at least the correct intention. If we change a file such that its dependencies are different, the order in which we unload namespaces should reflect the namespaces in memory, as they were before we changed the file.

When using c.t.n.dir/scan to detect and reload changed files this works correctly, at least most of the time.

When adding files to a new tracker for the first time, the old dependency graph is empty, so the unload order is undefined. This is arguably incorrect but effectively meaningless, since those namespaces have not yet been loaded.

In release 0.2.6, there was a bad commit which mistakenly changed c.t.n.dir/scan and scan-all to remove the files which have changed before adding them again. As a result, the dependencies of changed files were removed from the tracker's dependency graph before the unload order could be calculated, so unload order was always undefined. I have reverted that change — thanks for drawing my attention to it!

Now the unload order should be "correct" after scan or scan-all, except for the first time files are added to the tracker, when unload order is undefined.

The tracker doesn't currently have a way to distinguish between changed files which need unload+load and new files which only need load. Even if there were a way to distinguish between new and changed files, we can't be certain that a namespace has not been loaded by other means (e.g. require at the REPL) so it's safest to unload everything before reloading.

In general, unload order shouldn't matter at all. Clojure doesn't care when namespaces are removed: the Vars are still referenced from wherever they are used.

Comment by Andy Fingerhut [ 20/Sep/14 1:08 PM ]

OK, makes sense. Is it already documented anywhere that the unload order is independent of the dependencies after scan-all (and perhaps other calls)? I can create a separate ticket for that if you think it is worth adding such documentation.

It sounds like for the application I had in mind, the reverse of the load order is always defined, and is a correct unload order. You are welcome to close this ticket.





[TNS-19] deps-from-ns-decl can return nil Created: 06/Jun/14  Updated: 19/Sep/14  Resolved: 11/Jul/14

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Ambrose Bonnaire-Sergeant Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None


 Description   

I expect this to be #{}.

(ns-parse/deps-from-ns-decl '(ns cljs.core.typed "Internal functions for CLJS" (:refer-clojure :exclude [IFn]) (:require-macros [clojure.core.typed.bootstrap-cljs :as boot])))
;=> nil


 Comments   
Comment by Stuart Sierra [ 13/Jun/14 5:04 PM ]

Fixed in commit e9394727 on Git master. Please verify.

Comment by Stuart Sierra [ 11/Jul/14 4:42 PM ]

Merged on Git master; will be in 0.2.5 release.

Comment by Stuart Sierra [ 19/Sep/14 9:08 AM ]

Included in release 0.2.6





[TNS-18] Use linear time algorithm to calculate transitive dependencies, instead of current exponential time algorithm Created: 03/Jun/14  Updated: 19/Sep/14  Resolved: 11/Jul/14

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File demo-exponential-algo.patch     Text File linear-time-no-debug.patch    
Patch: Code

 Description   

The function clojure.tools.namespace.dependency/transitive takes exponential time for cases where there are an exponential number of paths in the DAG (directed acyclic graph) of dependencies. I discovered this when using tools.namespace on the namespaces of project core.typed, where the total time for finding a topological order was several minutes on a reasonably fast machine.

It is easy to calculate transitive dependents/dependencies in linear time in the worst case (linear in the number of dependency arcs in the DAG).



 Comments   
Comment by Andy Fingerhut [ 03/Jun/14 3:18 PM ]

Patch demo-exponential-algo.patch only adds some test cases that demonstrate the exponential running time of the current algorithm for calculating transitive dependencies.

Comment by Andy Fingerhut [ 03/Jun/14 3:20 PM ]

Patch linear-time-no-debug.patch is one way to implement transitive dependencies in linear time. It also introduces additional protocol functions, which may not be desired. I am definitely open to suggestions on what kind of change you would like to see here (assuming you want any changes at all).

Comment by Stuart Sierra [ 06/Jun/14 10:27 AM ]

This is a valuable improvement — thanks, Andy!

I don't want to add more methods to the protocol, but I don't see any way to avoid it right now. Ideally, transitive-dependencies would change to take a set, but that breaks backwards-compatibility.

I'll give it a week or so to think about naming:

  • transitive-dependencies-of-node-set is too long
  • transitive-dependencies-all ?
  • transitive-dependencies-set ?
  • all-transitive-dependencies ?
Comment by Stuart Sierra [ 11/Jul/14 4:42 PM ]

Merged on master as of commit 3f946380. Will be in release 0.2.5.

New functions are named transitive-dependencies-set and transitive-dependents-set.

Comment by Stuart Sierra [ 19/Sep/14 9:08 AM ]

Included in release 0.2.6





[TNS-17] doesn't track namespace when ns decl isn't first in file Created: 04/Feb/14  Updated: 19/Sep/14  Resolved: 11/Jul/14

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Trevor Wennblom Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: bug, enhancement


 Description   

This was first filed for ns-tracker https://github.com/weavejester/ns-tracker/issues/15 but I understand it's more specific to tools.namespace.

(I realize this is likely a very low priority, but it'd still be nice to see fixed if possible. Thanks!)


It appears ns-tracker tools.namespace will cease following a namespace when there's non-commented content before the namespace declaration.

works:

;-begin
(ns myproject.core)
(println "hi")
;-end

untracked — (any of the four possibilities when uncommented, for example):

;-begin
1
;;; (println "before ns")
;;; (println "loading" *file*)
;;; :abc
(ns myproject.core)
(println "hi")
;-end


 Comments   
Comment by Stuart Sierra [ 13/Jun/14 4:43 PM ]

What's the use case for having a file in which the ns declaration isn't the first non-comment form?

Comment by Trevor Wennblom [ 13/Jun/14 5:53 PM ]

In the case that triggered the issue for me, it can be useful to see the load-order of files based on the dependencies given in ns for debugging. That is, to print a message before and after the completion of loading the namespace dependencies.

I'm not sure how difficult or involved the issue would be to solve.

Comment by Andy Fingerhut [ 13/Jun/14 6:42 PM ]

Would the :verbose option to require perhaps give you the load-order info you want? Try something like this at a REPL, with a namespace you care about:

(require '[clojure.core.typed :as ct] :verbose)

In general, I think Stuart's concern would be questions like: How many forms would you expect tools.namespace to skip over before giving up? If it is a fixed number of forms before it gives up, why that number and not some other?

If it is based upon some other condition to give up looking for an ns form, what would that condition be?

Or would you expect it to be able to find the ns form in the body of a 'when' or 'if' form, and not at the top level?

The criteria can easily start to sound kind of arbitrary, and/or complex to implement.

Comment by Stuart Sierra [ 11/Jul/14 5:04 PM ]

Fixed on master as of commit 3c08b722. This turned out to be an easy change.

I wouldn't recommend putting anything before the ns declaration, but it's no longer required to be first. A file could also have multiple ns declarations, although clojure.tools.namespace.track will only look at the first one.

Comment by Stuart Sierra [ 19/Sep/14 9:08 AM ]

Included in release 0.2.6





[TNS-16] Some tests depend upon Clojure hash for ordering Created: 30/Jan/14  Updated: 19/Sep/14  Resolved: 31/Jan/14

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: File tns-16-v1.diff    

 Description   

In particular there are several that do a topological sort of a dependency graph, and compare the returned node sequence against a single node order. In general there are many correct results for a topological sort, and the implementation's return order depends upon Clojure's hash function, recently changed.



 Comments   
Comment by Andy Fingerhut [ 30/Jan/14 9:40 AM ]

Patch tns-16-v1.diff is one way to make the tests independent of Clojure's hash function. It implements a topo-check function that determines whether a particular node sequence order is in a valid dependency order, i.e. is a subsequence of at least one topological sorting of the nodes.

Comment by Stuart Sierra [ 31/Jan/14 9:09 AM ]

Given that the current tests only use very small, hand-drawn graphs, I'm going to go with the brute-force approach for now.

But thanks for this patch. I'd like to hold on to it for possible future use with generated graphs and/or test.check.

Comment by Stuart Sierra [ 19/Sep/14 9:08 AM ]

Included in release 0.2.6





[TNS-15] Can't find files when their absolute path contains a space Created: 12/Dec/13  Updated: 10/Jan/14  Resolved: 13/Dec/13

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Ryan Senior Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: File directories-with-a-space.diff    
Patch: Code

 Description   

The URLClassloader getURLs call is returning URLs that contain %20 instead of spaces. The current code calls .getPath, which leaves the %20 in there and ultimately can't find the directory. Changing the .getPath call to .toURI calls a different java.io.File constructor and fixed the problem for me.



 Comments   
Comment by Stuart Sierra [ 13/Dec/13 9:22 AM ]

Applied in commit 8f116ba8c6944acc347cba97db0244e247021016

Comment by Stuart Sierra [ 10/Jan/14 11:26 AM ]

Mark old resolved issues as 'closed'





[TNS-14] Wasted call to str in read-ns-decl Created: 02/Dec/13  Updated: 10/Jan/14  Resolved: 02/Dec/13

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Andy Fingerhut Assignee: Stuart Sierra
Resolution: Declined Votes: 0
Labels: None


 Description   

The function read-ns-decl (both copies in each of two namespaces) contain the expression:

(doto (read rdr) str)

This is equivalent to (read rdr), plus a wasted call to str whose return value is discarded.



 Comments   
Comment by Stuart Sierra [ 02/Dec/13 12:07 PM ]

The call to str was added to force the Clojure reader to throw exceptions immediately when it encounters a syntax error. See TNS-1

Comment by Andy Fingerhut [ 02/Dec/13 4:20 PM ]

Sorry for the noise. I was trying out pre-release version of Eastwood linter on as many contrib libraries as possible, and it found that odd-looking code. It didn't occur to me to look through the commit history to see when it became like it was, and why. Subtle.

Comment by Stuart Sierra [ 10/Jan/14 11:26 AM ]

Mark old resolved issues as 'closed'





[TNS-13] Refresh function not working properly Created: 17/Oct/13  Updated: 10/Jan/14  Resolved: 21/Oct/13

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Pierre Allix Assignee: Stuart Sierra
Resolution: Declined Votes: 0
Labels: None
Environment:

Linux, lein 2.2.0, Clojure 1.5.1



 Description   

It seems the clojure.tools.namespace.repl/refresh functions has a problem reloading my namespaces.

user=> (refresh)
:reloading (tikkba.core lacij.model.history tikkba.utils.dom tikkba.apps.svgbrowser lacij.model.graph-history tikkba.dom lacij.utils.core lacij.view.core lacij.view.rectnodeselection lacij.view.utils.style lacij.view.utils.text lacij.view.rectnodeview lacij.view.circlenodeview lacij.view.nodeview lacij.geom.distance lacij.view.straightedgeview tikkba.utils.xml lacij.view.nodelabelview lacij.model.edge lacij.view.graphview lacij.view.segmentededgeview lacij.model.node tikkba.swing lacij.model.graph lacij.view.edgelabelview lacij.edit.graph lacij.examples.swing carneades.engine.uuid carneades.engine.utils carneades.engine.statement carneades.engine.dublin-core carneades.engine.unify carneades.engine.argument carneades.engine.argument-generator carneades.config.config carneades.engine.theory.zip carneades.engine.theory.namespace carneades.engine.theory carneades.project.admin carneades.database.db carneades.database.argument-graph carneades.web.pack lacij.examples.simple carneades.engine.argument-graph carneades.engine.translation carneades.engine.theory.translation carneades.database.export carneades.database.import carneades.policy-analysis.web.core carneades.engine.search carneades.engine.argument-evaluation carneades.engine.dung carneades.engine.caes carneades.database.evaluation carneades.engine.aspic carneades.engine.policy carneades.policy-analysis.web.logic.questions carneades.engine.dialog carneades.policy-analysis.web.controllers.reconstruction lacij.opt.annealing carneades.xml.graphml.export carneades.engine.theory.namespace-test lacij.geom.intersect lacij.layouts.core lacij.layouts.randomlayout lacij.layouts.utils.position carneades.maps.format-statement carneades.engine.triplestore carneades.web.license-analysis.model.triplestore carneades.maps.lacij-params lacij.layouts.naivelayout lacij.layouts.hierarchicallayout lacij.layouts.radiallayout carneades.maps.subset-ag lacij.layouts.layout carneades.maps.lacij-export carneades.engine.ask carneades.engine.sandbox carneades.engine.argument-builtins carneades.engine.argument-construction carneades.engine.shell carneades.policy-analysis.web.logic.askengine carneades.maps.lacij carneades.web.license-analysis.model.analysis carneades.web.license-analysis.model.debug-analysis carneades.policy-analysis.web.views.pages carneades.database.case carneades.policy-analysis.web.controllers.policy-simulation carneades.policy-analysis.web.routes carneades.web.outline carneades.web.license-analysis.routes.service carneades.web.vote carneades.policy-analysis.web.jetty carneades.xml.caf.export carneades.web.project carneades.web.info carneades.web.service carneades.analysis.web.routes-dev carneades.analysis.web.routes-selfexe tikkba.utils.test.dom lacij.edit.dynamic lacij.examples.listener carneades.engine.dnf carneades.engine.test-dnf carneades.analysis.web.routes-war carneades.policy-analysis.web.main lacij.examples.styles lacij.examples.textwrapping lacij.examples.undoredo carneades.owl.owl carneades.owl.import-axioms lacij.examples.radiallayout2 carneades.engine.argument-edit carneades.project.admin-test carneades.web.liverpool-schemes carneades.engine.owl.rule lacij.examples.circle lacij.examples.def carneades.maps.graphviz lacij.examples.hierarchicallayout2 lacij.examples.layout carneades.engine.test-evaluation carneades.engine.test-theory lacij.examples.radiallayout tikkba.examples.dynamic carneades.analysis.web.system carneades.owl.import-language carneades.owl.import carneades.engine.triplestore-test carneades.web.test.service carneades.xml.lkif.lkif tikkba.examples.output-string lacij.examples.dynamic lacij.examples.hierarchicallayout tikkba.examples.writefile tikkba.examples.image carneades.owl.import-language-test lacij.examples.display-xml lacij.examples.bug tikkba.examples.analemma carneades.engine.test-scheme lacij.test.core carneades.engine.test-unify carneades.database.test.admin carneades.engine.test-statement lacij.examples.radialloop carneades.xml.lkif.import lacij.examples.custom tikkba.examples.core tikkba.examples.svgattr lacij.examples.graphdeps user tikkba.test.core carneades.engine.owl.reasoner carneades.policy-analysis.web.logic.server-properties carneades.xml.lkif.export carneades.xml.lkif.validator)
:error-while-loading carneades.engine.argument-generator
#<Exception java.lang.Exception: No namespace: carneades.engine.argument>

user=> (require 'carneades.engine.argument)
nil
user=> (require 'carneades.engine.argument-generator)

Exception No namespace: carneades.engine.argument clojure.core/refer (core.clj:3832)
user=>

Just (require 'carneades.engine.argument-generator) from a newly started REPL works so there is a problem with refresh.

I use checkouts directory in Lein and various use and requires forms.



 Comments   
Comment by Stuart Sierra [ 17/Oct/13 11:02 AM ]

Do you have any AOT-compiled namespaces in your project?

`refresh` does not work with AOT-compiled namespaces.

Comment by Pierre Allix [ 21/Oct/13 4:20 AM ]

I had one aot namespace, which I removed. I still get one strange error:

user=> (compile 'carneades.web.service)
carneades.web.service
user=> (refresh)
:reloading (tikkba.core lacij.model.history tikkba.utils.dom tikkba.apps.svgbrowser lacij.model.graph-history tikkba.dom lacij.utils.core lacij.view.core lacij.view.rectnodeselection lacij.view.utils.style lacij.view.utils.text lacij.view.rectnodeview lacij.view.circlenodeview lacij.view.nodeview lacij.geom.distance lacij.view.straightedgeview tikkba.utils.xml lacij.view.nodelabelview lacij.model.edge lacij.view.graphview ...)
:error-while-loading carneades.web.service
#<ExceptionInInitializerError java.lang.ExceptionInInitializerError>
user=> (pst)
IllegalStateException Alias dc already exists in namespace eu.markosproject.licensing.oss-licensing-theory, aliasing carneades.engine.dublin-core
clojure.lang.Namespace.addAlias (Namespace.java:224)
clojure.core/alias (core.clj:3870)
clojure.core/load-lib (core.clj:5387)
clojure.core/apply (core.clj:619)
clojure.core/load-libs (core.clj:5413)
clojure.core/apply (core.clj:619)
clojure.core/require (core.clj:5496)
eu.markosproject.licensing.oss-licensing-theory/eval19048/loading-4910auto---19049 (oss_licensing_theory.clj:4)
eu.markosproject.licensing.oss-licensing-theory/eval19048 (oss_licensing_theory.clj:4)
clojure.lang.Compiler.eval (Compiler.java:6619)
clojure.lang.Compiler.eval (Compiler.java:6608)
clojure.lang.Compiler.load (Compiler.java:7064)
nil

The namespace eu.markosproject.licensing.oss-licensing-theory is loaded dynamically with load-file. Could this be the source of the problem?

Comment by Stuart Sierra [ 21/Oct/13 3:46 PM ]

JIRA is not a good venue for debugging problems with one specific application. If you have an isolated test case, please create a new ticket with instructions to reproduce the failure. Until then, please direct all questions to the Clojure mailing list: https://groups.google.com/forum/#!forum/clojure

To answer the latest question: yes, clojure.tools.namespace.repl/refresh is incompatible with other code which generates or loads namespaces on-the-fly.

Comment by Stuart Sierra [ 10/Jan/14 11:26 AM ]

Mark old resolved issues as 'closed'





[TNS-12] Duplicate jar-file? definition in find.clj Created: 16/Sep/13  Updated: 10/Jan/14  Resolved: 17/Sep/13

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Alex Coventry Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: patch

Attachments: Text File 0001-Remove-duplicate-of-jar-file-fn.patch    
Patch: Code

 Description   

There are two copies of the jar-file? function in find.clj.



 Comments   
Comment by Stuart Sierra [ 17/Sep/13 7:24 AM ]

Fixed in commit 0e4fe44be8ded594d0374d4cc21d6f42a697c18e

Comment by Stuart Sierra [ 10/Jan/14 11:26 AM ]

Mark old resolved issues as 'closed'





[TNS-11] c.t.n.p/deps-from-ns-decl does not like (:use [some-root-ns module1 module2]) Created: 02/Jul/13  Updated: 10/Jan/14  Resolved: 02/Jul/13

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Thomas Christensen Assignee: Stuart Sierra
Resolution: Duplicate Votes: 0
Labels: None
Environment:

Clojure 1.5.1 ; Clojure.tools.namespace 0.2.3



 Description   

I am experiencing issues using the c.t.n.r/refresh on my project.

I believe that I have tracked down the issue to the way deps-from-ns-decl parses :use; that is: it is different from Clojure's.

I have several places used ns-forms of the shape:

(ns some-root-ns.module1
       (:use [some-root-ns module2 module3])

Where Clojure would load some-root-ns.module2 and some-root-ns.module3.

However, this is what deps-from-ms-decl gets:

(clojure.tools.namespace.parse/deps-from-ns-decl '(ns some-root-ns.module1
       (:use [some-root-ns module2 module3])))
;; => #{some-root-ns}

Note that while it is of course easily fixed by using this ns-form:

(clojure.tools.namespace.parse/deps-from-ns-decl '(ns some-root-ns.module1
       (:use [some-root-ns.module2])
       (:use [some-root-ns.module3])))
;; => #{some-root-ns.module3 some-root-ns.module2}

It would probably be desirable to be equivalent with Clojure's behavior.



 Comments   
Comment by Stuart Sierra [ 02/Jul/13 7:44 AM ]

c.t.namespace.parse does not currently support all the varied argument forms that Clojure's use and require will accept.

In general, c.t.namespace.parse adheres to the docstrings of the use and require functions, which state that prefixed namespaces are written as a list, not a vector. The use and require functions do not enforce this, leading to widespread variation in usage.

This is a duplicate of TNS-9.

Until it is resolved, you can write your prefixed namespaces as a list, like:

(ns some-root-ns.module1
  (:use (some-root-ns module2 module3)))

In general, I recommend against using prefix lists in ns declarations because they are harder to identify using text-based search tools.

Comment by Stuart Sierra [ 10/Jan/14 11:26 AM ]

Mark old resolved issues as 'closed'





[TNS-10] Stack overflow on cyclic dependency Created: 15/Jun/13  Updated: 10/Jan/14  Resolved: 17/Jun/13

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Ambrose Bonnaire-Sergeant Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None


 Description   

If a cyclic dependency is introduced between a namespace and itself, it results in a StackOverflow when calculating transitive-dependencies.

(transitive-dependencies (-> (graph)
                             (depend 'two 'two))
                         'two)
; => StackOverflow

I believe clojure.tools.namespace.dependency/transitive should maintain a set of seen namespaces and check if we have already seen the namespace at each step.



 Comments   
Comment by Stuart Sierra [ 15/Jun/13 10:33 AM ]

Already fixed in 0.2.4-SNAPSHOT by forbidding cyclic dependencies: https://github.com/clojure/tools.namespace/commit/85e73af04fdb497db1600ef3bf21aef5ed676619

Comment by Stuart Sierra [ 10/Jan/14 11:26 AM ]

Mark old resolved issues as 'closed'





[TNS-9] Warn or error when parsing illformed prefix list Created: 14/Jun/13  Updated: 10/Jan/14  Resolved: 19/Jul/13

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Ambrose Bonnaire-Sergeant Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: File parse-fail.diff    

 Description   

Currently a ns entry (:require [foo [bar]]) where the prefix list is a vector is silently ignored.

Preferred behaviour is a warning or an error.



 Comments   
Comment by Ambrose Bonnaire-Sergeant [ 15/Jun/13 11:27 PM ]

The patch throws an exception on several ill-formed expressions.

Comment by Stuart Sierra [ 19/Jul/13 8:30 AM ]

Fixed in 0.2.4-SNAPSHOT

Commits e37221fecaf71ac0ca7e3022e3ee2cadbfbc1a45 and 930833dce8949a0154382038ac5fde85c5fb0abd

Comment by Stuart Sierra [ 10/Jan/14 11:26 AM ]

Mark old resolved issues as 'closed'





[TNS-8] move-ns changes the timestamps on files whose content doesn't change Created: 14/Jun/13  Updated: 10/Jan/14  Resolved: 19/Jul/13

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Simon Katz Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: enhancement


 Description   

The summary says it all.
It's a minor annoyance; fixing it would be trivial.



 Comments   
Comment by Stuart Sierra [ 19/Jul/13 8:29 AM ]

Fixed in 0.2.4-SNAPSHOT

Commit 9b43f4a07e4d7264368d64a0dbbd3a2bc3b04097

Comment by Stuart Sierra [ 10/Jan/14 11:26 AM ]

Mark old resolved issues as 'closed'





[TNS-7] Stack overflow on refresh after circular dependency detected Created: 17/Apr/13  Updated: 17/Apr/13

Status: Open
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

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

tools.namespace 0.2.3



 Description   
user> (refresh)
Exception Circular dependency between com.example.foo and com.example.bar  clojure.tools.namespace.dependency.MapDependencyGraph (dependency.clj:74)
user> (refresh)
StackOverflowError   clojure.lang.PersistentHashMap$BitmapIndexedNode.index (PersistentHashMap.java:576)
user> (clojure.repl/pst)
StackOverflowError 
	clojure.lang.PersistentHashMap$BitmapIndexedNode.index (PersistentHashMap.java:576)
	clojure.lang.PersistentHashMap$BitmapIndexedNode.find (PersistentHashMap.java:676)
	clojure.lang.PersistentHashMap$ArrayNode.find (PersistentHashMap.java:396)
	clojure.lang.PersistentHashMap.valAt (PersistentHashMap.java:152)
	clojure.lang.PersistentHashMap.valAt (PersistentHashMap.java:156)
	clojure.lang.RT.get (RT.java:645)
	clojure.tools.namespace.dependency/transitive (dependency.clj:52)
	clojure.tools.namespace.dependency/transitive/fn--7043 (dependency.clj:51)
	clojure.core.protocols/fn--6022 (protocols.clj:79)
	clojure.core.protocols/fn--5979/G--5974--5992 (protocols.clj:13)
	clojure.core/reduce (core.clj:6177)
	clojure.tools.namespace.dependency/transitive (dependency.clj:52)
nil





[TNS-6] Attempt to reload deleted file Created: 14/Dec/12  Updated: 14/Dec/12

Status: Open
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 0
Labels: None


 Description   

I can't identify the exact circumstances, but I have seen events where a source code file has been deleted but clojure.tools.namespace.repl/refresh still tries to reload it. Because the file doesn't exist, there's an exception when you try to load it, so you're stuck.






[TNS-5] Allow any valid .clj* source file to be parsed/analysed Created: 01/Nov/12  Updated: 07/Sep/14

Status: Open
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Max Penet Assignee: Stuart Sierra
Resolution: Unresolved Votes: 0
Labels: enhancement

Attachments: Text File e3cd6d1fa6e0c900bc1086e4a93bbc9cb343a820.patch    

 Description   

This broadens the allowed file types to anything ending with #"\.clj.?$", meaning this would work for clj, cljs, cljc, cljx and possibly other Clojure implementations with their own extension in the future.

This allows libraries such as codox (and possibly autodoc) to work with ClojureScript and others implementations without any modification.

Note: My CA is on the way, I sent it a week ago, I am not sure if it arrived yet (it was sent from Switzerland with normal mail, with an ETA of 1 week).



 Comments   
Comment by Max Penet [ 02/Nov/12 6:51 AM ]

CA received it seems (I am listed on http://clojure.org/contributing ).

Comment by Stuart Sierra [ 02/Nov/12 3:23 PM ]

I'm not sure about this. If you're only using c.t.n.find in isolation, it's fine. But if you're using code-reloading and c.t.n.repl, it could incorrectly try to reload .cljs files in JVM Clojure.

We really need [Feature Expressions]http://dev.clojure.org/display/design/Feature+Expressions or something like it to get away from multiple file extensions.

Until then, I think it has to be optional. I don't know how best to achieve this. The APIs in c.t.n.repl and c.t.n.dir are not amenable to extension. I'll think about it.

Comment by Max Penet [ 06/Nov/12 6:11 PM ]

True, I didn't realize that.

Maybe using a dynamic var to hold the regex pattern (or a predicate?) could be a reasonable solution in the meantime, this would allow to rebind it in the case of codox & similar libs.
I don't really "like" to use dynamic vars (or regexes!), but in this case it might make sense.
Not to mention it would also allow more flexibility on projects where you don't want to have your clj codoxed, but only your cljs for instance.

Comment by Max Penet [ 24/Nov/12 7:47 AM ]

Any thoughts on my last comment/edit? I don't think there is a single doc lib that works with cljs at the moment, it is a bit painful to be honest.

Comment by Stuart Sierra [ 24/Nov/12 4:25 PM ]

Yes, I think a dynamic var would be OK. However, I would like to know of a real use case, not just a potential one.

Comment by Max Penet [ 13/Dec/12 2:40 AM ]

The idea was to be able to use codox on cljs files, I tried locally but there are other problems with this approach to be able to get to cljs vars metadata. So in the end I think you were right, maybe it's better to wait for feature expressions for this.





[TNS-4] Eliminate several uses of reflection in tools.namespace Created: 28/Oct/12  Updated: 10/Jan/14  Resolved: 28/Oct/12

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File tns-4-eliminate-reflection-v1.txt    

 Description   

There are several reflection warnings when compiling tools.namespace, all of which can be eliminated with fairly straightforward additions of type hints.



 Comments   
Comment by Andy Fingerhut [ 28/Oct/12 8:08 PM ]

tns-4-eliminate-reflection-v1.txt dated Oct 28 2012 adds the necessary type hints to eliminate reflection warnings in tools.namespace.

Comment by Stuart Sierra [ 28/Oct/12 9:14 PM ]

Patch applied. Thanks!

Comment by Stuart Sierra [ 10/Jan/14 11:26 AM ]

Mark old resolved issues as 'closed'





[TNS-3] refresh-dirs not used Created: 08/Oct/12  Updated: 10/Jan/14  Resolved: 08/Oct/12

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None


 Description   

Submitted by Mika Raento as a GitHub pull request: https://github.com/clojure/tools.namespace/pull/1



 Comments   
Comment by Stuart Sierra [ 08/Oct/12 4:59 PM ]

Fixed in commit bc9d5c1a6f191070a425b04feda9e2d3c2eb6928

https://github.com/clojure/tools.namespace/commit/bc9d5c1a6f191070a425b04feda9e2d3c2eb6928

Comment by Stuart Sierra [ 10/Jan/14 11:26 AM ]

Mark old resolved issues as 'closed'





[TNS-2] Add function to parse dependencies from namespace declarations Created: 16/May/12  Updated: 10/Jan/14  Resolved: 16/May/12

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File TNS-2-0001.patch    
Patch: Code

 Comments   
Comment by Stuart Sierra [ 10/Jan/14 11:26 AM ]

Mark old resolved issues as 'closed'





[TNS-1] Workaround to Clojure 1.2 reader bug Created: 14/Dec/11  Updated: 10/Jan/14  Resolved: 24/Apr/12

Status: Closed
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Sam Ritchie Assignee: Stuart Sierra
Resolution: Completed Votes: 2
Labels: None
Environment:

Mac OS X, Clojure 1.2.1, Leiningen 1.6.2


Attachments: Text File 0001-Workaround-to-Clojure-1.2-reader-bug.patch    
Patch: Code and Test

 Description   

The clojure 1.2 reader will allow invalid forms like {:key} to be read in, and only throw an exception on printing. Currently clojure.tools.namespace calls (read rdr) within a try form; the bug means that this particular type of error is never caught. This patch forces the reader to try and resolve with str, allowing clojure.tools.namespace to catch and bury the error.

I was running into this with moustache templates from lein-newnew on the classpath; these contain namespace headers that look like (ns name.core). This would cause (clojure.tools.namespace/find-namespaces-on-classpath) to fail when printing its results but not when actually running.



 Comments   
Comment by Sam Ritchie [ 14/Dec/11 4:55 PM ]

Funny, jira picked up the moustache markup. a bad namespace looks like (ns { { name } } . core).

Comment by Sam Ritchie [ 11/Jan/12 3:59 PM ]

Ping – Stuart, any thoughts on this?

Comment by Stuart Sierra [ 11/Jan/12 6:09 PM ]

Why should tools.ns do this? If the syntax is wrong, it's wrong.

Comment by Sam Ritchie [ 11/Jan/12 6:43 PM ]

Because without this patch, it's impossible to catch and bury errors from invalid reader syntax. I believe this comes from a bug in the reader that was fixed with 1.2.

Comment by Stuart Sierra [ 27/Jan/12 9:44 AM ]

Declined. It is not the responsibility of this library to catch errors in old versions of Clojure.

Comment by Stuart Sierra [ 24/Apr/12 2:01 PM ]

Reopening because this is still a visible issue for some libraries. I still don't like it, but I'm going to include it.

Comment by Stuart Sierra [ 24/Apr/12 2:04 PM ]

Patch applied.

Comment by Sam Ritchie [ 24/Apr/12 2:07 PM ]

Great, thanks!

Comment by Stuart Sierra [ 10/Jan/14 11:26 AM ]

Mark old resolved issues as 'closed'





[TMACRO-6] with-symbol-macros fails to preserve set sortedness Created: 04/Jul/14  Updated: 04/Jul/14

Status: Open
Project: tools.macro
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: François-René Rideau Assignee: Konrad Hinsen
Resolution: Unresolved Votes: 0
Labels: None
Environment:

clojure 1.6.0



 Description   

with-symbol-macros fails to preserve set sortedness of the literals it encounters in the code it has expanded. This notably causes it to improperly munge the code from the case macro.

See the full discussion on the clojure mailing-list: https://groups.google.com/forum/#!topic/clojure/HdmkjLcyqWQ






[TMACRO-5] Misplaced doc string for function protected? Created: 23/Dec/13  Updated: 23/Dec/13

Status: Open
Project: tools.macro
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Andy Fingerhut Assignee: Konrad Hinsen
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File tmacro-5-v1.diff    

 Description   

Not a big deal, but I was testing a pre-release version of the Eastwood Clojure lint tool on many libraries, and found this issue among many others.



 Comments   
Comment by Andy Fingerhut [ 23/Dec/13 6:34 PM ]

Patch tmacro-5-v1.diff simply moves the doc string to the correct location.





[TMACRO-4] README.md still says 0.1.2 is latest stable release Created: 17/Nov/13  Updated: 17/Nov/13

Status: Open
Project: tools.macro
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Andy Fingerhut Assignee: Konrad Hinsen
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Perhaps that is correct, since 0.1.5 is not considered stable? If not, it would be nice to update it.






[TMACRO-3] Handling of namespaced symbols Created: 04/Sep/13  Updated: 10/Sep/13

Status: Open
Project: tools.macro
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Dmitry Groshev Assignee: Konrad Hinsen
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Here is a snippet:

(def spec-map
  {:int {:type 'ints}})

(defmacro specialize [type body]
  `(symbol-macrolet [type$ ~(-> spec-map type :type)]
     ~(w/postwalk
       (fn [form]
         (if-let [tag (-> form meta :tag)]
           (if (= tag 'type$)
             (with-meta form {:tag (-> spec-map type :type)})
             form)
           form))
       (mexpand-all body))))

(defmacro caster [x]
  `(type$ ~x))

(specialize :int
 (defn test-getter [x]
   (let [^type$ x x]
     (prn "my type" type$)
     (caster x)
     (aget x 0))))

It's broken, the error is "Unable to resolve symbol: type$ in this context". Why is it so? Let's macroexpand it:

(clojure.tools.macro/symbol-macrolet 
    [clojure.core.matrix.impl.ndarray-magic/type$ ints] 
    (def test-getter 
      (fn* ([x] (let* [x x] (prn "my type" type$) (clojure.core.matrix.impl.ndarray-magic/type$ x) (aget x 0))))))

Now, the reason is obvious: symbol-macrolet expects namespaced symbol, and in expanded form "type$" is once namespaced and once not. I think that symbol-macrolet can (should?) ignore namespacing here.



 Comments   
Comment by Konrad Hinsen [ 10/Sep/13 4:09 AM ]

After a bit of thought, I think the right solution is to treat symbol macros like ordinary symbols in evaluation. This means that global definitions (defsymbolmacro) use namespaced symbols, whereas local symbol definitions (symbol-macrolet) accept only plain symbols and raise an exception for qualified symbols, just like let does.

With those rules, the way tools.macro handles namespaces in correct code is correct, but it doesn't do the required error handling because it lets you symbol-macrolet qualified symbols.

Note also that according to those rules, your example code is not correct. You have to write

(symbol-macrolet [~'type$ ...] ...)

and

(defmacro caster [x]
  `(~'type$ ~x))

although you might then prefer to write the latter as

(defmacro caster [x]
   (list 'type$ x))

but that's a matter of taste.





[TMACRO-2] protect let-bound symbols from macrolet expansion Created: 26/Nov/12  Updated: 27/Nov/12  Resolved: 27/Nov/12

Status: Resolved
Project: tools.macro
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Tom Jack Assignee: Konrad Hinsen
Resolution: Completed Votes: 1
Labels: bug, patch

Attachments: Text File 0001-Don-t-apply-macro-expansion-to-protected-forms.patch     File macrolet-protect.diff    
Patch: Code and Test

 Description   

As discussed here: https://groups.google.com/d/msg/clojure-dev/-/UheAzkyI_WcJ

Patch macrolet-protect.diff fixes the issue and provides a test.



 Comments   
Comment by Tom Jack [ 26/Nov/12 2:55 PM ]

D'oh, patch macrolet-protect.diff has a bug — it doesn't macroexpand-1 if the symbol is protected.

Comment by Tassilo Horn [ 27/Nov/12 1:38 AM ]

This patch supersedes Tom's patch as discussed on the clojure-dev mailinglist. See

Message-ID: <CADygAw4ArNq4Z2=ZJmT6MwkBw160ShJmfQwoFEh4VOiwxfjDKQ@mail.gmail.com>

for reference.

The patch also adds letfn* as a form introducing protected symbols. That one was completely missing.

I added test cases for both let as well as letfn.

Comment by Konrad Hinsen [ 27/Nov/12 8:40 AM ]

Patch applied: https://github.com/clojure/tools.macro/commit/19c8197e10079f04e55d81c26884b9248762c2ca

Thanks!





[TMACRO-1] Preserve metadata for (at least) unexpanded forms Created: 02/Apr/12  Updated: 02/Apr/12

Status: Open
Project: tools.macro
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Stephen Compall Assignee: Konrad Hinsen
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Clojure 1.3.0, tools.macro 0.1.1, algo.monads 0.1.0



 Description   

In algo.monads:

(with-monad blah some-code...)

causes all of some-code to report as being on the line of the with-monad form. Could, perhaps, expand-all copy over metadata from the original form, at least in the common case that the form-at-point was not itself expanded?






[TLOG-13] tools.logging ns form :require and :use subforms unrecognized by tools.namespace Created: 23/Aug/14  Updated: 20/Sep/14  Resolved: 20/Sep/14

Status: Closed
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Andy Fingerhut Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File tlog-13-1.patch    

 Description   

Cross-reference: TNS-21

I am not sure whether this is something that should be changed in tools.logging or tools.namespace. To be sure, at most one of them should change, if either. Stuart Sierra has mentioned in TNS-21 that the ns doc string shows :require :use etc. subforms always in parentheses, and not vectors. It might be best to change tools.logging's ns form.

tools.namespace expects (:require ...) and (:use ...) subforms of the ns form to be in lists, not vectors, otherwise it ignores them.

tools.logging has ns forms that use vectors like [:require ...] and [:use ...], and are thus ignored by tools.namespace.

The Clojure compiler seems to handle these dependencies just fine, but this may be just another case where it accepts more things than it is documented to accept.



 Comments   
Comment by Andy Fingerhut [ 29/Aug/14 7:42 PM ]

Patch tlog-13-1.patch dated Aug 29 2014 simply replaces [] with () around subforms of ns forms.

Comment by Alexander Taggart [ 20/Sep/14 1:35 PM ]

https://github.com/clojure/tools.logging/commit/67d2fa8411e1f226bf1e77f3e5f6326a00e5b954





[TLOG-12] Reflection Warnings when using tools.logging Created: 30/Apr/13  Updated: 07/Jun/14  Resolved: 13/May/14

Status: Closed
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Trevor Bernard Assignee: Alexander Taggart
Resolution: Not Reproducible Votes: 0
Labels: None

Attachments: Text File 0001-Fix-TLOG-7-Remove-reflection-warnings.patch    
Waiting On: Trevor Bernard

 Description   

A few of my projects are still reflecting using tools.logging 0.2.6 and slf4j-log4j12 "1.7.2".

lein check produces the following output:

...
Reflection warning, curator_clj/zookeeper.clj:1:1 - reference to field isTraceEnabled can't be resolved.
Reflection warning, curator_clj/zookeeper.clj:1:1 - reference to field isDebugEnabled can't be resolved.
Reflection warning, curator_clj/zookeeper.clj:1:1 - reference to field isInfoEnabled can't be resolved.
Reflection warning, curator_clj/zookeeper.clj:1:1 - reference to field isWarnEnabled can't be resolved.
Reflection warning, curator_clj/zookeeper.clj:1:1 - reference to field isErrorEnabled can't be resolved.
Reflection warning, curator_clj/zookeeper.clj:1:1 - reference to field isErrorEnabled can't be resolved.
...



 Comments   
Comment by Alexander Taggart [ 30/Apr/13 5:37 PM ]

I've made a quick test project: https://github.com/ataggart/reflect-log-test

Cloning that repo, running lein check yields no reflection warnings.

Please provide a way for me to reproduce what you see.

Comment by Trevor Bernard [ 30/Apr/13 5:46 PM ]

Forking that project and will try to find a simple test case that reproduces it.





[TLOG-11] tools.logging doesn't AOT Created: 22/Mar/13  Updated: 30/Apr/13  Resolved: 30/Apr/13

Status: Closed
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Allen Rohner Assignee: Alexander Taggart
Resolution: Declined Votes: 0
Labels: None

Waiting On: Allen Rohner

 Description   

tools.logging does not work if used in a project that is AOT'd.

To reproduce, in a project using tools.logging:

lein jar
lein run

I can workaround it in my project by including

(when *compile-files*
  (compile 'clojure.tools.logging)
  (compile 'clojure.tools.logging.impl))


 Comments   
Comment by Alexander Taggart [ 30/Apr/13 5:11 PM ]

It's not clear to me what "does not work" means.

I've made a quick test project: https://github.com/ataggart/aot-log-test

Cloning that repo, running lein jar then lein run yields the output I would expect.

Further details appreciated.

Comment by Allen Rohner [ 30/Apr/13 6:32 PM ]

This isn't a bug; User Error.

I thought I had fully isolated the bug in my app's codebase, but I forgot about an :injection that we have that jams a debug macro into clojure.core.

for posterity, it was caused by doing

(binding [*ns* (find-ns 'clojure.core)]
  (require 'clojure.tools.logging)
  (defmacro inspect
    "prints the expression '<name> is <value>', and returns the value"
    [value]
    `(do
       (let [value# (quote ~value)
             result# ~value]
         (println value# "is" (with-out-str (clojure.pprint/pprint result#)))
         (clojure.tools.logging/infof "%s is %s" value# result#)
         result#))

Moving the require call to inside the macroexpansion resolved the bug. (And yes, I'm moving to https://github.com/dgrnbrg/spyscope now that I'm aware of it)

Sorry to waste your time, Alex.

Comment by Alexander Taggart [ 30/Apr/13 7:06 PM ]

No worries.

On a related note, do you see any utility in keeping the spy macro? It's pretty much what you're doing. Do you use inspect for logging or as an in-repl debugging tool? Perhaps spy makes more sense in tools.trace.

Comment by Allen Rohner [ 30/Apr/13 7:12 PM ]

TBH, I didn't know about spy. Also, my inspect macro predates clojure.tools.logging, and might even predate clojure.contrib.logging, if you can believe that.

I use inspect as a debugging tool. The main value to jamming it into clojure.core is that I can use it in any file, without having to stop and muck with the ns declaration. Therefore if I was going to use anything else, it would need to have that property.





[TLOG-10] Double-evaluation of arguments Created: 04/Feb/13  Updated: 04/Feb/13  Resolved: 04/Feb/13

Status: Closed
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Chris Perkins Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None
Environment:

tools.logging 0.2.5-SNAPSHOT


Attachments: File double-eval.diff    

 Description   

The expansion of the log macros can double-evaluate the first argument.

For example, the following call:

(log/info (expensive-call foo) bar)

Expands to this:

(let*
[logger_278auto_
(get-logger logger-factory #<Namespace scratch.core>)]
(if (enabled? logger_278auto_ :info)
(if (instance? java.lang.Throwable (expensive-call foo))
(log*
logger_278auto_
:info
(expensive-call foo)
(print-str bar))
(log*
logger_278auto_
:info
nil
(print-str (expensive-call foo) bar)))))

Note the calls to "expensive-call" both in the instance check and in the log* call.



 Comments   
Comment by Alexander Taggart [ 04/Feb/13 10:28 PM ]

Fixed in 0.2.6.





[TLOG-9] spy macro (or new spy macro) that accepts a message Created: 08/Dec/12  Updated: 07/Jun/14  Resolved: 07/Jun/14

Status: Closed
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Dave Sann Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None


 Description   

I think it might be useful to have spy be able to take a message to be logged in addition to the data. (or have a similar macro showing an optional message instead of the code).

(spy "doing x" (blah))
(spy :doing-x (blah) ; because it is easier to type sometimes

with spy this may conflict with the log-level.

I find this sort of function with a message useful and use it quite often. Much more often than plain logging. Generally - I would't log the code unless it logged file and line number too.



 Comments   
Comment by Alexander Taggart [ 07/Jun/14 7:10 PM ]

Added spyf, which takes a format string.

https://github.com/clojure/tools.logging/commit/91bc737ac37e1dbf8f4d658aa304154afac7a1dc





[TLOG-8] README.md still refers to using version 0.2.3 of tools.logging Created: 24/Nov/12  Updated: 04/Feb/13  Resolved: 04/Feb/13

Status: Closed
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Trevor Bernard Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File TLOG-8.patch    

 Description   

The latest version is 0.2.4 and should reflected in the README



 Comments   
Comment by Alexander Taggart [ 04/Feb/13 10:27 PM ]

Fixed in 0.2.6.





[TLOG-7] Reflection warnings Created: 16/Apr/12  Updated: 07/Jun/14  Resolved: 05/May/12

Status: Closed
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Sean Corfield Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Fix-TLOG-7-Remove-reflection-warnings.patch     Text File TLOG-7.1.patch    

 Description   

Reflection warning, clojure/tools/logging.clj:270 - reference to field isTraceEnabled can't be resolved.
Reflection warning, clojure/tools/logging.clj:270 - reference to field isDebugEnabled can't be resolved.
Reflection warning, clojure/tools/logging.clj:270 - reference to field isInfoEnabled can't be resolved.
Reflection warning, clojure/tools/logging.clj:270 - reference to field isWarnEnabled can't be resolved.
Reflection warning, clojure/tools/logging.clj:270 - reference to field isErrorEnabled can't be resolved.
Reflection warning, clojure/tools/logging.clj:270 - reference to field isFatalEnabled can't be resolved.
Reflection warning, clojure/tools/logging.clj:270 - call to trace can't be resolved.
Reflection warning, clojure/tools/logging.clj:270 - call to debug can't be resolved.
Reflection warning, clojure/tools/logging.clj:270 - call to info can't be resolved.
Reflection warning, clojure/tools/logging.clj:270 - call to warn can't be resolved.
Reflection warning, clojure/tools/logging.clj:270 - call to error can't be resolved.
Reflection warning, clojure/tools/logging.clj:270 - call to fatal can't be resolved.



 Comments   
Comment by Alexander Taggart [ 05/May/12 3:26 PM ]

Adds type-hints to all implementation-wrapping fns.

Comment by Trevor Bernard [ 23/Apr/13 4:39 PM ]

A few of my projects are still reflecting using tools.logging 0.2.6 and slf4j-log4j12 "1.7.2". I've attached a patch that I've started using to fix this issue.

Comment by Trevor Bernard [ 23/Apr/13 4:41 PM ]

I'm a registered contributor to clojure.

http://clojure.org/contributing

Comment by Andy Fingerhut [ 24/Apr/13 12:33 PM ]

Trevor, since this ticket was already closed and marked as completed some time ago, it is probably best to create a new ticket and attach your patch to that. I know the issue is similar to this existing ticket, but best to have new tickets for new work.





[TLOG-6] tools.logging does not log to stdout when run from swank-clojure Created: 07/Feb/12  Updated: 07/Jun/14  Resolved: 07/Feb/12

Status: Closed
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Trevor Bernard Assignee: Alexander Taggart
Resolution: Declined Votes: 0
Labels: None
Environment:

tools.logging "0.2.3", Mac OS X Lion, emacs 23.4, swank-clojure 1.3.4, slime 20100404



 Description   

Trying to evaluate (info "Hello, World!") doesn't print to stdout when evaluating from within emacs. It works if I `lein run` however.



 Comments   
Comment by Trevor Bernard [ 07/Feb/12 8:44 PM ]

It works when I use the inferior-lisp process but not in slime

Comment by Alexander Taggart [ 07/Feb/12 9:07 PM ]

Configuring swank/slime to work with wherever your log implementation writes its output is not a tools.logging issue.





[TLOG-5] Improvement of sample code in README.md Created: 14/Oct/11  Updated: 07/Jun/14  Resolved: 17/Oct/11

Status: Closed
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Jingguo Yao Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None


 Description   

README.md uses the following code to show the usage of tools.logging.

(ns example.core
  (:use [clojure.tools.logging :only (info error)]))

(defn divide [one two]
  (try
    (info "Calculated" (/ one two))
    (catch Exception ex
      (error ex "There was an error in calculation"))))

If log4j.rootLogger in the given log4j.properties is updated to "ERROR, A1". "(divide 10 0)" won't evaluate "(error ex "There was an error in calculation")". The reason is that INFO logging level is lower than ERROR logging level. So the info macro will not evaluate "(/ one two)" since the specific INFO logging level is not in effect.

New tools.logging users often expect "(divide 10 0)" to produce logging with ERROR level. They are confused with the above behaviour. So I suggest to make some change to the sample code. Maybe the following code:

(ns example.core
  (:use [clojure.tools.logging :only (info error)]))

(defn divide [one two]
  (try 
    (info "Calculating" one two)
    (/ one two)
    (catch Exception ex
      (error ex "There was an error in calculation"))))





[TLOG-4] Provided implementations of logging protocols fail. Created: 29/Sep/11  Updated: 07/Jun/14  Resolved: 29/Sep/11

Status: Closed
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Blocker
Reporter: Alexander Taggart Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File fix-broken-extend.patch    
Patch: Code and Test

 Description   

Example of error when trying to call a Logger protocol function:

java.lang.IllegalArgumentException: No implementation of method: :enabled? of protocol:
#'clojure.tools.logging.impl/Logger found for class: org.apache.log4j.Logger

The bug is the result of combining syntax-quoting with extend-type, e.g.:

`(do
   (extend-type org.apache.commons.logging.Log
     Logger
     (enabled? [logger# level#] ... )))

The syntax-quote turns the above enabled? into clojure.tools.logging/enabled? protocol function name, and then the extend-type turns that into the keyword :clojure.tools.logging/enabled?, which does not match the expected :enabled?.

Attached patch fixes this by using extend maps rather than extend-type. Also adds some tests.



 Comments   
Comment by Alexander Taggart [ 29/Sep/11 3:36 PM ]

Committed:

https://github.com/clojure/tools.logging/commit/f9cb0a7232d3a11cb7e74e4c13c3adc054e7e7a2





[TLOG-3] Move implementation-specific details out of main ns Created: 06/Jul/11  Updated: 07/Jun/14  Resolved: 07/Jul/11

Status: Closed
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alexander Taggart Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File refactor-namespaces.patch    
Patch: Code and Test
Approval: Ok

 Description   

The protocols which allow the main functions/macros to adapt to implementation specific logging libraries do not need to be in the clojure.tools.logging namespace since they are not intended to be used by consumers.

Plan:

  • Move protocols into clojure.tools.logging.impl
  • Move implementation-specific code (e.g., the extentions to the protocols) into separate namespaces, e.g., clojure.tools.logging.log4j.


 Comments   
Comment by Alexander Taggart [ 07/Jul/11 6:49 PM ]

https://github.com/clojure/tools.logging/commit/42dfba84a373e52a4e80af6467e47761e6f20678





[TLOG-2] Change "log" semantics to "logger" semantics Created: 05/Jul/11  Updated: 07/Jun/14  Resolved: 06/Jul/11

Status: Closed
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Laurent Petit Assignee: Laurent Petit
Resolution: Completed Votes: 0
Labels: None

Attachments: File tools.logging-TLOG-2-v2.diff    
Patch: Code and Test
Approval: Ok

 Description   

The relevant patch is named tools.logging-TLOG-2-v2.diff

Changes "log" semantics to "logger" semantics.

Log protocol becomes LoggerProtocol
LogFactory protocol becomes LoggerFactoryProtocol
log-factory becomes logger-factory
impl-get-log becomes impl-get-logger
log function argument name becomes logger function argument name
log# macro autosym becomes logger# macro autosym



 Comments   
Comment by Alexander Taggart [ 06/Jul/11 3:00 PM ]

Committed to master:
https://github.com/clojure/tools.logging/commit/0a5f9bd3148cf078dd0b62aece5f92a970b5aba3





[TLOG-1] Non-optimized, no-exception path for logp calls log* with wrong arity Created: 01/Jun/11  Updated: 07/Jun/14  Resolved: 01/Jun/11

Status: Closed
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Alexander Taggart Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File TLOG-1.patch    
Patch: Code and Test

 Description   

Per Laurent Petit



 Comments   
Comment by Alexander Taggart [ 01/Jun/11 11:34 AM ]

https://github.com/clojure/tools.logging/commit/687c1833c59ad1204c2a41cf0ed1f53be4a0fa8d





[TGEN-5] Defspec leaks :tag from args into generated code Created: 13/Dec/13  Updated: 13/Dec/13

Status: Open
Project: test.generative
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Stuart Halloway
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Don-t-leak-tag-from-defspec-args-into-generated-code.patch    
Patch: Code

 Description   

See http://dev.clojure.org/jira/browse/TANAL-24 and https://groups.google.com/d/msg/clojure-dev/hRZFuaiB_50/mzKLirgZWmUJ for more info






[TGEN-4] Typo in defspec docstring Created: 11/Mar/13  Updated: 11/Mar/13

Status: Open
Project: test.generative
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Ambrose Bonnaire-Sergeant Assignee: Stuart Halloway
Resolution: Unresolved Votes: 0
Labels: None


 Description   

The docstring for `defspec` misspells "metdata".






[TGEN-3] binding *seed* does not change the basis Created: 13/Oct/12  Updated: 03/Feb/13  Resolved: 03/Feb/13

Status: Closed
Project: test.generative
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Ben Smith-Mannschott Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None


 Description   

This is what I'm observing:

user=> (binding [gen/*seed* 7] (gen/uniform))
-6880819372607069048
user=> (binding [gen/*seed* 7] (gen/uniform))
5065957067589062886

This is what I'm expecting:

user=> (binding [gen/*seed* 7] (gen/uniform))
-4967725919621401576
user=> (binding [gen/*seed* 7] (gen/uniform))
-4967725919621401576

The work-around is to bind *rnd* to a new java.util.Random constructed appropriately:

(binding [gen/*rnd* (java.util.Random. 7)] (gen/uniform))

I'm not sure: Is this is a bug in the doc string of *seed*? Is the bug the very existence of *seed*? (*seed* is never used.) Should the doc-string for *rnd* suggest something like the code above as a way to get a consistent basis? Should generators provide a with-seed macro to do this for us?



 Comments   
Comment by Stuart Halloway [ 03/Feb/13 7:13 AM ]

this is fixed on master in the data.generators project





[TGEN-2] ASCII DEL (0x7f, 127) is not a printable character Created: 13/Oct/12  Updated: 14/Oct/12  Resolved: 14/Oct/12

Status: Resolved
Project: test.generative
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Ben Smith-Mannschott Assignee: Ben Smith-Mannschott
Resolution: Completed Votes: 0
Labels: None

Attachments: File ascii-127-is-not-printable.diff    
Patch: Code
Approval: Accepted

 Description   

ASCII 127 is a control character.

Dec Hex    Dec Hex    Dec Hex  Dec Hex  Dec Hex  Dec Hex   Dec Hex   Dec Hex  
  0 00 NUL  16 10 DLE  
  1 01 SOH  17 11 DC1  
  2 02 STX  18 12 DC2  
  3 03 ETX  19 13 DC3  
  4 04 EOT  20 14 DC4  
  5 05 ENQ  21 15 NAK  
  6 06 ACK  22 16 SYN  
  7 07 BEL  23 17 ETB  
  8 08 BS   24 18 CAN  
  9 09 HT   25 19 EM   
 10 0A LF   26 1A SUB  
 11 0B VT   27 1B ESC  
 12 0C FF   28 1C FS   
 13 0D CR   29 1D GS   
 14 0E SO   30 1E RS   
 15 0F SI   31 1F US                                                 127 7F DEL


 Comments   
Comment by Ben Smith-Mannschott [ 14/Oct/12 11:14 AM ]

Fixed by a73c11e3 on test.generative.





[TGEN-1] generators/shuffle violates contract of Comparable Created: 13/Oct/12  Updated: 14/Oct/12  Resolved: 14/Oct/12

Status: Resolved
Project: test.generative
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Ben Smith-Mannschott Assignee: Ben Smith-Mannschott
Resolution: Completed Votes: 0
Labels: None
Environment:

Mac OS X 10.8.2
java version "1.7.0_06"
Java(TM) SE Runtime Environment (build 1.7.0_06-b24)
Java HotSpot(TM) 64-Bit Server VM (build 23.2-b09, mixed mode)


Attachments: File fisher-yates-shuffle.diff    
Patch: Code
Approval: Accepted

 Description   

JDK 7's sort function is stricter about verifying that implementations of the comparison function actually obey their contract. It will even helpfully throw an exception when this is not the case:

java.lang.IllegalArgumentException: Comparison method violates its general contract!

The root cause is this gem:

generators.clj
(defn shuffle
      "Shuffle coll"
      [coll]
      (sort-by (fn [_] (long)) coll))

Which does not work on JDK7 since TimSort is clever enough to detect that the ordering function is behaving inconsistently, resulting in the previously mentioned exception.

Also, this is just a bad idea:

A variant of the above method that has seen some use in languages that support sorting with user-specified comparison functions is to shuffle a list by sorting it with a comparison function that returns random values. However, this is an extremely bad method: it is very likely to produce highly non-uniform distributions, which in addition depends heavily on the sorting algorithm used.

(wikipedia)



 Comments   
Comment by Ben Smith-Mannschott [ 14/Oct/12 11:13 AM ]

Fixed by 5a59bf0f on test.generative.





[TEMJVM-8] try expressions in statement position are broken Created: 24/Jul/14  Updated: 25/Jul/14  Resolved: 25/Jul/14

Status: Closed
Project: tools.emitter.jvm
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None


 Description   
(eval '(ƒ [] (do
                (try
                  1
                  (catch Exception e))
                1)))
Illegal exception table range in class file clojure/tools/emitter/jvm$fn__23378
  [Thrown class java.lang.ClassFormatError]





[TEMJVM-7] lift loops in expr context into methods Created: 21/Jul/14  Updated: 24/Jul/14  Resolved: 24/Jul/14

Status: Closed
Project: tools.emitter.jvm
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None


 Description   

see http://dev.clojure.org/jira/browse/CLJ-701






[TEMJVM-6] Bindings analysis defect Created: 28/May/14  Updated: 28/May/14  Resolved: 28/May/14

Status: Closed
Project: tools.emitter.jvm
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Reid McKenzie Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File ERROR.txt    

 Description   

Initially encountered in the attached snippet. It seems like letfn's parallel bindings are not correctly handled by the emitter/analyzer.



 Comments   
Comment by Nicola Mometto [ 28/May/14 4:52 PM ]

fixed in the tools.analyzer collect pass: https://github.com/clojure/tools.analyzer/commit/60e271ca7066c59d04d683c7c403045d3e048669





[TEMJVM-5] Optimize constant metadata emission Created: 27/Apr/14  Updated: 27/Apr/14

Status: Open
Project: tools.emitter.jvm
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Nicola Mometto
Resolution: Unresolved Votes: 0
Labels: None





[TEMJVM-4] Double args compilation on protocol-invoke Created: 15/Feb/14  Updated: 15/Feb/14  Resolved: 15/Feb/14

Status: Closed
Project: tools.emitter.jvm
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None


 Description   

This leads to problems if an args is e.g. a fn



 Comments   
Comment by Nicola Mometto [ 15/Feb/14 12:11 PM ]

Fixed: https://github.com/clojure/tools.emitter.jvm/commit/e62d0a5b77f6dce1ff2bcd53cdb96ffb7349dabc





[TEMJVM-3] Optimize emission of empty collections Created: 28/Jan/14  Updated: 29/Jan/14  Resolved: 29/Jan/14

Status: Closed
Project: tools.emitter.jvm
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Nicola Mometto Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None


 Comments   
Comment by Nicola Mometto [ 29/Jan/14 12:12 PM ]

https://github.com/clojure/tools.emitter.jvm/commit/b2747ef7bad4805644d24767b2af175c73ac0457





[TEMJVM-2] Fix class naming in type defs Created: 27/Jan/14  Updated: 27/Jan/14  Resolved: 27/Jan/14

Status: Closed
Project: tools.emitter.jvm
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None


 Description   
clojure.tools.emitter.jvm> (eval '(byte-array 1))
clojure.lang.Numbers.byte_array(Ljava/lang/Object;)Ljava/lang/[B;
  [Thrown class java.lang.NoSuchMethodError]


 Comments   
Comment by Nicola Mometto [ 27/Jan/14 7:57 PM ]

fixed: https://github.com/clojure/tools.emitter.jvm/commit/af17ed4705c82407efa554b15369b3b2998c1a7d





[TEMJVM-1] Fix classloader handling Created: 23/Jan/14  Updated: 27/Jan/14  Resolved: 27/Jan/14

Status: Closed
Project: tools.emitter.jvm
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Blocker
Reporter: Nicola Mometto Assignee: Nicola Mometto
Resolution: Completed Votes: 0
Labels: None


 Description   

This currently throws:

(let [a (reify)] (fn [] a))


 Comments   
Comment by Nicola Mometto [ 27/Jan/14 1:43 PM ]

Fixed: https://github.com/clojure/tools.emitter.jvm/commit/8a5de2b884eaa5ce9b3afc44a6d50a147b528bad





[TCLI-10] potential incorrect parsing Created: 23/Jul/14  Updated: 10/Aug/14

Status: Open
Project: tools.cli
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Travis Camechis Assignee: Gareth Jones
Resolution: Unresolved Votes: 0
Labels: None


 Description   

If i have these options defined

(def cli-options [ ["-i" "--interactive" "interactive cli mode"]
["-H" "--help" "show help"]
["h" "-host host" "hostname"
["-u" "--user user" "username to login to rulegate"]])

if I enter a command as follows

myprog -h myhost -u foo

my option output looks like { :host myhost :user foo }

however if I leave the host value off
{:host -u}

It appears to be treating the -u as the value for -h. I would expect a missing argument error. Is this the correct behavior?



 Comments   
Comment by John Walker [ 10/Aug/14 3:01 PM ]

I am not able to reproduce this behavior. Would you create a gist or use code tags?





[TCLI-9] Support multiple validations Created: 08/Feb/14  Updated: 22/Apr/14

Status: Open
Project: tools.cli
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Jean Niklas L'orange Assignee: Gareth Jones
Resolution: Unresolved Votes: 0
Labels: None


 Description   

In many cases, I have input arguments where I want to perform multiple validations, and return different error messages based upon that. Would it be reasonable to have multiple validation functions, or being able to have a validation function returning nil if no errors exists, and an error string if not?

If this seems like an interesting idea, I believe that the easiest and simplest solution would be to support a single validation function returning the actual error message. The reationale for this is that multiple validations may require a complex rule system (ordering? should it short-circuit, or should it return multiple error messages?), and as such, it's better to have those rules evident in the validation function.



 Comments   
Comment by Sung Pae [ 13/Feb/14 10:35 PM ]

My apologies for this delayed response. I thought that I would be emailed on creation of new issues, but this does not appear to be the case. This is the second time this has happened, so I will clearly have to rig something up to keep on top of this board.

> In many cases, I have input arguments where I want to perform multiple validations, and return different error messages based upon that. Would it be reasonable to have multiple validation functions, or being able to have a validation function returning nil if no errors exists, and an error string if not?

> If this seems like an interesting idea, I believe that the easiest and simplest solution would be to support a single validation function returning the actual error message. The reationale for this is that multiple validations may require a complex rule system (ordering? should it short-circuit, or should it return multiple error messages?), and as such, it's better to have those rules evident in the validation function.

The single largest reason I opted against implementing validations like this was that I dislike the corresponding linguistic and syntactic awkwardness of inverting the logic:

:validate [pred "Must satisfy pred"] ;; Error message is optional

vs

:validate #(when-not (pred %) "Must satisfy pred")

The first approach looks like a standard assertion, while the second requires an explicit branch and must return a truthy value to register a failure.¹

Now, the second example is more powerful for the reasons you outline, but since the vast majority of command line option arguments are semantically simple values such as numbers, filenames, and hostnames, I chose to optimize for the common case in order to have the terser syntax.

That said, I am happy to be convinced otherwise. Could you please provide an example of an option specification where returning different error messages for different failures is significantly better than something like the following?

["o" "-option EXAMPLE"
:validate [#(and (foo? %) (bar? %) (baz? %))
"Must be a foo that is also a bar and a baz"]]

I am currently of the mind that a per-option error summary is sufficient for an advanced user interface like the command line, and becomes complete with a link to a documentation URL (or man page).

Thank you.

¹ I usually name functions of the second type `validation-errors` instead of `validate` because I find this confusing:

(when-not (validate input) "This seems awkward.")

Comment by Jean Niklas L'orange [ 14/Feb/14 9:42 AM ]

Hello Sung,

No worries about the response time.

It's not necessarily the fact that it is more verbose, just that I would like to give good, descriptive error messages back. While it's easy for each of the different errors, I think it's better to say exactly what the problem is. For instance, if I want to take in an input directory path, I want to ensure that is is a legal path, and if the file already exists, it must be a directory:

["-i" "--input-directory DIR"
 :validate 
 #(if-not (ok-path? %)
    (format "Input directory path '%s' is malformed" %)
    (let [f (File. %)]
      (if (and (.exists f) (not (.isDirectory f)))
        (format "Input directory '%s' is a file, not a directory" %))))]

However, I can understand that such functionality can be a bit over the top for a CLI library. As long as you've compared/considered the different possibilities, I am content with whatever choice you take.

Comment by Sung Pae [ 17/Feb/14 11:17 PM ]

Saying yes to this kind of enhancement should be quite easy, but I only have a single reservation: adding a second type of validation function that is a logical inverse of the existing type seems confusing.

What do you think about using exceptions to carry error messages?

https://gist.github.com/guns/9065033
(gaah I don't know how to use JIRA at all)

The existing implementation already catches Exceptions, throws away the error message, and returns nil. This is more verbose than fn -> optarg -> nil | String, but it is aligned to the current logical convention.

Comment by Sung Pae [ 22/Apr/14 12:21 AM ]

Hello Jean,

I've implemented your first suggestion (support multiple validations) and pushed it to master. I think it's a nice compromise.

From the patch header:

This is an implemenation (sic) of the first suggestion, multiple validation
functions.

The :validation entry can now have multiple entries, which are
partitioned into pairs, then split into the vector entries
:validation-fn and :validation-msg. It would be nice to have
a pluralized key name here, but we decline this for backwards
compatibility.

Non-collection values of :validate-fn and :validate-msg continue to be
supported by conditional wrapping.

Example:

    [["-f" "--file PATH"
      :parse-fn io/file
      :validate [#(.exists %) "File does not exist"
                 #(.isFile %) "Path is not a regular file"
                 #(.canRead %) "File is unreadable"]]]

Addresses TCLI-9.

https://github.com/clojure/tools.cli/commit/56ecd5f305d444bbf6dcd28f4f4adce58ebc0de4





[TCLI-8] Correct test that trivially always passes Created: 31/Dec/13  Updated: 02/Jan/14  Resolved: 02/Jan/14

Status: Resolved
Project: tools.cli
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Andy Fingerhut Assignee: Sung Pae
Resolution: Completed Votes: 0
Labels: None

Attachments: File tcli-8-v1.diff    

 Description   

The following tools.cli test always trivially passes, because (= expr) is always true:

(is (= {:verbose false}))

Found using pre-release version of Eastwood Clojure lint tool.



 Comments   
Comment by Andy Fingerhut [ 31/Dec/13 1:57 AM ]

Patch tcli-8-v1.diff corrects the test.

Comment by Sung Pae [ 02/Jan/14 1:33 AM ]

Thank you, patch applied!





[TCLI-7] summarize throws when called with an empty sequence of options Created: 17/Dec/13  Updated: 02/Jan/14  Resolved: 02/Jan/14

Status: Resolved
Project: tools.cli
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Hugo Duncan Assignee: Sung Pae
Resolution: Completed Votes: 0
Labels: None


 Description   

When calling opt-parse with an empty sequence of options, summarize throws an exception.

actual: clojure.lang.ArityException: Wrong number of args (1) passed to: core$map
 at clojure.lang.AFn.throwArity (AFn.java:437)
    clojure.lang.RestFn.invoke (RestFn.java:412)
    clojure.lang.AFn.applyToHelper (AFn.java:161)
    clojure.lang.RestFn.applyTo (RestFn.java:132)
    clojure.core$apply.invoke (core.clj:619)
    clojure.tools.cli$summarize.invoke (cli.clj:375)

https://github.com/hugoduncan/tools.cli/compare/feature;fix-summary-no-options?expand=1



 Comments   
Comment by Sung Pae [ 02/Jan/14 1:32 AM ]

Thank you, patch applied!

I am sorry for the delay; I mistakenly thought I would receive email notifications on new issues so have not been polling JIRA.

I hope to ameliorate this soon.





[TCLI-6] Merge optparse-clj and increase modularity Created: 20/Nov/13  Updated: 10/Dec/13  Resolved: 10/Dec/13

Status: Resolved
Project: tools.cli
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Sung Pae Assignee: Sung Pae
Resolution: Completed Votes: 0
Labels: enhancement


 Description   

On Sun 25 Aug 2013 at 09:05:15PM -0500, gaz jones wrote:

> Hey, i am the current maintainer of tools.cli - i have very little
> time to make any changes to it at the moment (kids ). I'm not sure
> what the process is for adding you as a developer or transferring
> ownership etc but if I'm happy to do so as I have no further plans for
> working on it.

Hello Gareth,

Sorry for delay in action. I submitted my Clojure CA that week and have
been casually awaiting confirmation ever since.

Only today have I noticed that my name has appeared on
http://clojure.org/contributing, so I suppose that is confirmation
enough.

This is my proposal for tools.cli:

  • Merge the CLI arguments lexer from github.com/guns/optparse-clj to
    support GNU option parsing conventions.

Examples:
https://github.com/guns/optparse-clj#features

guns.cli.optparse/tokenize-arguments:
https://github.com/guns/optparse-clj/blob/master/src-cljx/guns/cli/optparse.cljx#L25-74

GNU options parsing spec:
https://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html

  • Adapt tools.cli/cli to use the arguments lexer, then freeze it to
    maintain backwards compatibility.
  • Create a new function tools.cli/parse-opts, based largely on the
    design of guns.cli.optparse/parse, that supports the following
    features:
  • Granular options specification map.

Given the following setup:

(ns my.ns
...)

(def cli-options
[["s" "-server HOSTNAME" "Remote server"
:default (java.net.InetAddress/getByName \"example.com\")
:default-desc "example.com"
:parse-fn #(java.net.InetAddress/getByName %)
:assert-fn (partial instance? Inet4Address)
:assert-msg "%s is not an IPv4 host"]
[...]
])

A call to (clojure.tools.cli/compile-option-specs cli-options)
will result in the following PersistentArrayMap:

{option-id ; :server
{:short-opt String ; "-s"
:long-opt String ; "--server"
:required String ; "HOSTNAME"
:desc String ; "Remote server"
:default Object ; #<Inet4Address example.com/93.184.216.119>
:default-desc String ; "example.com"
:parse-fn IFn ; #(InetAddress/getByName %)
:assoc-fn IFn ; assoc
:assert-fn IFn ; (partial instance? Inet4Address)
:assert-msg String ; "%s is not an IPv4 host"
}}

The optspec compiler will verify uniqueness of option-id,
:short-opt, and :long-opt values and throw an AssertionError on
failure.

The optspec map is a PAM to preserve options ordering for summary
generation.

  • Customizable options summary.

tools.cli/parse-opts will return an options summary string to
the caller. Printing the summary with a banner will be the
responsibility of the caller.

The default options summary will look like:

-p, --port NUMBER 8080 Remote port
-s, --server HOSTNAME example.com Remote server
--detach Detach and run in the background
-h, --help

The above format can be changed by supplying an optional :summary-fn
flag that will receive the optspec map values from above and return
a summary string. The default summary-fn will be a public var.

This addresses TCLI-3.

  • Optional in-order options processing, with trailing options parsed
    by default.

This is necessary for managing different option sets for
subcommands. Indirectly addresses TCLI-5.

  • No runtime exceptions.

While parse-opts will throw an AssertionError for duplicate
option-id, :short-opt, and :long-opt values during compilation,
option parsing errors will no longer throw exceptions.

Instead, a map of {option-id error-string} will be provided, or nil
if there are no errors. Correspondingly, parse-opts will have the
following function prototype:

parse-opts:
[argument-seq [& option-vectors] & compiler-flags]
->
{:options PersistentArrayMap ; optspec above
:arguments PersistentVector ; non-optarg arguments
:summary String ; options summary produced by summary-fn
:errors {Keyword String} ; error messages by option
}

The expected usage of this function will look like:

(def usage-banner
"Usage: my-program [options] arg1 arg2\n\nOptions:\n")

(defn exit [status msg]
(println msg)
(System/exit status))

(defn -main [& argv]
(let [{:keys [options arguments summary errors]}
(cli/parse-opts argv cli-options :in-order true)]
(when (:help options)
(exit 0 (str usage-banner summary)))
(when (not= (count arguments) 2)
(exit 1 (str usage-banner summary)))
(when errors
(exit 1 (string/join "\n" (cons "The following errors have occured:\n"
(vals errors)))))
(apply my-program! options arguments)))

  • ClojureScript support.

github.com/guns/optparse-clj currently supports CLJS/node.js via
the cljx preprocessor and an extern file for the Closure Compiler
`process` namespace.

If this is desirable for tools.cli, a similar setup will be applied,
except that I will attempt to avoid cljx for easier hacking.

Comments are appreciated, and I am eager to amend this proposal to gain
community acceptance.

Cheers,
Sung Pae



 Comments   
Comment by Sung Pae [ 20/Nov/13 6:27 PM ]

A more nicely formatted version of the above is available here:

https://gist.github.com/guns/7573819/raw

EDIT:

There is an error in the function prototype above. The actual return value should be:

```
{:options {Keyword Object} ; {:server "my-server.com"}
:arguments PersistentVector ; non-optarg arguments
:summary String ; options summary produced by summary-fn
:errors {Keyword String} ; error messages by option
}
```

Specifically, :options will of course return a PHM of option keywords to their parsed values.

Comment by Gareth Jones [ 20/Nov/13 9:22 PM ]

Sounds good. One thing to add - there is an open issue on github to allow a single dash as an argument (https://github.com/clojure/tools.cli/issues/17) e.g. to indicate you wish to use stdin to read from. This seems to be used as a convention in several *nix cli utilities (such as tar for example). I was intending on adding a patch to support that by simply allowing '-' to be a valid argument name. If you can work that (or something equivalent) into your changes too that would be great!

Thanks,

Gareth

Comment by Sung Pae [ 22/Nov/13 3:57 PM ]

Sorry for the delayed response; I am finding JIRA a bit confusing.

In my experience with options parsers from other languages, '-' as
stdin is typically left to the discretion of the caller as there are
situations where you might want '-' to be a normal argument (e.g. for
describing arithmetic expressions or ranges).

Therefore I believe the proper answer to issue 17 is for the user to
call

`(replace {"-" *in*} rest-args)`

on the remaining arguments vector, and not to bake this behavior into
tools.cli.

I will be happy to make this argument to kyptin on Github if you find
this reasoning persuasive.

Meanwhile, I will wait patiently for someone on the core team to respond
to your post on clojure-dev.

Thank you!

Sung Pae

Comment by Sung Pae [ 10/Dec/13 1:20 PM ]

clojure.tools.cli/parse-opts completes the conceptual merge of optparse-clj.





[TCLI-5] Optionally allow unkown options Created: 25/Oct/13  Updated: 02/Jan/14  Resolved: 02/Jan/14

Status: Resolved
Project: tools.cli
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Hugo Duncan Assignee: Sung Pae
Resolution: Declined Votes: 0
Labels: None

Attachments: File allow_unkown_options    

 Description   

When building a cli with subcommands, it can be useful to allow passing through unkown options as arguments, so they can be processed in a subcommand.

The current API doesn't allow for passing any configuration to the `cli` command, so this adds an `allow-unknown-opts?` var, which when bound to true, will pass through unknown options.



 Comments   
Comment by Hugo Duncan [ 31/Oct/13 8:03 AM ]

A patched version of tools.cli is available on clojars as `[com.palletops/tools.cli "0.2.4"]`

Comment by Sung Pae [ 10/Dec/13 1:50 PM ]

Hello,

In the next version of tools.cli, parse-opts accepts an :in-order option to process arguments sequentially until the first unknown token.

The README has an explanation:

https://github.com/clojure/tools.cli#in-order-processing-for-subcommands

This is not the same as passing through unknown options, but it is the convention I am familiar with in large programs with subcommands.

Is this solution satisfactory for you?

The only times I have ever implemented a pass-through option for an options parser was for programs that wrap other command line programs. In these cases, the wrapper had a possibly intersecting superset of options, and I wanted to provide access to the underlying command's options without specifying them in my wrapper.

This worked okay, but the resulting options interface was complex, and since the relationship of the wrapper and underlying command was implicit, a bit fragile.

A major problem with the superset parser was that all unknown options were assumed to require arguments. This is necessary because it is impossible to tell whether `-ab` is meant to be interpreted as `-a -b` or `-a "b"` without knowing whether `-a` requires an argument. We can work around this by requiring that pass-through options always specify option arguments with an equal sign, but now we have introduced subtle differences for different tiers of options.

In contrast to all that, the subcommand options processing style of the git program and others like it allow many intersecting sets of options to be present in a single command invocation simply by following the easily understood convention:

cmd [cmd-opts] subcmd1 [subcmd1-opts]

I hope you find this argument persuasive, but I am happy to discuss it further!

Comment by Hugo Duncan [ 17/Dec/13 9:45 AM ]

The new `:in-order` option provides a better solution.

Comment by Sung Pae [ 02/Jan/14 1:36 AM ]

Thank you for your thoughts on this.





[TCLI-4] Paramters without value. Created: 11/Sep/13  Updated: 15/Dec/13  Resolved: 15/Dec/13

Status: Resolved
Project: tools.cli
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Ivan Boldyrev Assignee: Sung Pae
Resolution: Completed Votes: 0
Labels: None


 Description   

Currently there is only one type of parameters without value: boolean parameters introduced with :flag true.

But what if parameters define actions that have to be performed, and some actions need no argument? Then presence "-no-action1" with "-action" and help message would be strange at least, if not misleading.



 Comments   
Comment by Paul Stadig [ 02/Oct/13 8:01 AM ]

This is basically the case with a help option. If you specify

["-h" "--help" "Display usage" :default false :flag true]

as an option, then you get banner like this

Switches               Default     Desc                    
 --------               -------     ----                    
 -h, --no-help, --help  false       Display usage
Comment by Sung Pae [ 10/Dec/13 1:19 PM ]

In the upcoming version of tools.cli, parse-opts does not print a default value for boolean options or options without a specified :default.

In addition, option summaries may be customized with the :summary-fn option:

cf. https://github.com/clojure/tools.cli/issues/13#issuecomment-30258137

Comment by Sung Pae [ 15/Dec/13 3:14 PM ]

I am going to close this issue as it has been addressed by 0.3.0.

Thank you!





[TCLI-3] Change contract to provide access to banner on parse error Created: 09/Feb/13  Updated: 15/Dec/13  Resolved: 15/Dec/13

Status: Resolved
Project: tools.cli
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Philip Aston Assignee: Sung Pae
Resolution: Completed Votes: 1
Labels: None


 Description   

If a user provides an invalid option, most applications would want to display the banner.

tools.cli throws an Exception with minimal information, and (banner-for) is marked private.

The exception could be replaced with one that provides access to the banner, but I think it would be better (easier to use, more idiomatic?) to change the contract so that the banner is always returned. Given the exception string is also useful, perhaps change cli to return a map with keys [:options :extra-args :banner :parse-failure]?



 Comments   
Comment by Gunnar Völkel [ 08/Mar/13 4:19 AM ]

I agree with the reporter. Throwing an exception for invalid arguments ("[...] is not a valid argument") is not appropriate since you usually want to display that error message along with the banner.

Comment by Sung Pae [ 10/Dec/13 1:26 PM ]

In the upcoming 0.3.0 release, parse-opts does not throw exceptions on parse errors, but collects them into an errors vector which the caller checks to see if parsing was successful.

parse-opts does have a :post assertion on the uniqueness of :id, :short-opt, and :long-opt entries in the compiled option maps, but these errors are meant to help development, and may even be switched off by disabling assert at compile time.

Comment by Sung Pae [ 15/Dec/13 3:15 PM ]

I am going to close this issue as it has been addressed by 0.3.0.

Thank you!





[TCLI-2] Allow caller-supplied parse-fn Created: 11/Nov/12  Updated: 06/Aug/13  Resolved: 06/Aug/13

Status: Resolved
Project: tools.cli
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Pierre-Yves Ritschard Assignee: Gareth Jones
Resolution: Completed Votes: 0
Labels: enhancement

Attachments: File TCLI-2.diff    
Patch: Code and Test

 Description   

As for :parse-fn, a function can be supplied, this
is useful for standard use-cases such as: -vv, or when
you want to build a list from values.

PR: https://github.com/clojure/tools.cli/pull/11



 Comments   
Comment by Andy Fingerhut [ 11/Nov/12 12:38 PM ]

Pierre-Yves, there are instructions for creating patches under the headings "Development" and "Adding patches" on this page: http://dev.clojure.org/display/design/JIRA+workflow

Submissions to this module do require the author to sign a CA. Instructions here: http://clojure.org/contributing

Comment by Pierre-Yves Ritschard [ 11/Apr/13 9:48 AM ]

Suggested Patch

Comment by Pierre-Yves Ritschard [ 11/Apr/13 9:48 AM ]

Hello, now that I'm a registered contributor, I attached a file as suggested in the workflow wiki

Comment by Gareth Jones [ 06/Aug/13 1:13 PM ]

Applied this patch, and will release as 0.2.4 shortly. Thanks.

Comment by Gareth Jones [ 06/Aug/13 1:13 PM ]

Applied





[TCLI-1] Do not include keys when no value provided and no :default Created: 18/May/12  Updated: 24/Aug/12  Resolved: 24/Aug/12

Status: Closed
Project: tools.cli
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File TCLI-1-0001.patch     Text File TCLI-1-0002.patch    
Patch: Code and Test

 Description   

I was trying to use tools.cli in conjunction with a configuration map
loaded from a file, and use clojure.core/merge to combine the results.

This didn't work because tools.cli always uses a default value of
nil, even when no default value is specified. The nil always
overrides defaults from another source.

Example before the patch:

(def my-defaults
  {:foo 1})

(merge my-defaults
       (first (clojure.tools.cli/cli
               [] ; no arguments given
               ["--foo"]))) ; no default specified
;;=> {:foo nil}

This enhancement modifies tools.cli to completely omit arguments which
have no default specified and no value given on the command line.

After the patch, the above example returns:

;;=> {:foo 1}


 Comments   
Comment by Stuart Sierra [ 20/Jul/12 7:41 AM ]

New patch file 0002 fixes syntax error in the previous patch; updated description to show correct results.

Comment by Stuart Sierra [ 24/Aug/12 8:31 AM ]

Patch applied in commit https://github.com/clojure/tools.cli/commit/f9d92395cb788dd08cb144035a9d5fd8706d10b5





[TCHECK-45] Add ns to such-that example in README.md Created: 22/Sep/14  Updated: 22/Sep/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Ryan Fowler Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File add-ns-for-such-that.diff    
Patch: Code

 Description   

The clojure.test integration example is missing a "gen/" in front of the such-that function call.

The attached patch just makes it slightly easier to copy/paste the example into a project.






[TCHECK-44] for-all should support nesting Created: 22/Sep/14  Updated: 22/Sep/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Michael Sperber Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File for-alls-nest.diff    
Patch: Code and Test

 Description   

Haskell QuickCheck allows for-all expressions to nest. This is useful when there are dependencies between generated values. test.check should allow this, too.

Currently, nested for-alls always succeed, which is somewhat pernicious.

I've added a patch that implements this.






[TCHECK-43] Test.check lacks a generator for all floats and floats in a range Created: 17/Sep/14  Updated: 17/Sep/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Adam Clements Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: enhancement

Attachments: File float-generators.diff    
Patch: Code and Test

 Description   

Test.check lacks generators for floats and float ranges. I have implemented a float generator which shrinks to zero based on gen/ratio and also a float-in-range which is bounded by size.






[TCHECK-42] Interruptibility Created: 03/Sep/14  Updated: 03/Sep/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Gary Fredericks Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File TCHECK-42.patch    

 Description   

Both the main test loop and the shrink loop should have checks for thread interrupts

Patch coming imminently.



 Comments   
Comment by Gary Fredericks [ 03/Sep/14 3:55 PM ]

Attached TCHECK-42.patch.





[TCHECK-41] Add helpers for deprecating generators Created: 01/Sep/14  Updated: 01/Sep/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: John Walker Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File deprecation-helpers.diff    
Patch: Code

 Description   

It should be clearly communicated to the user when generators are deprecated. This patch supplies helpers for easily deprecating generators.

def-throwing-generator receives the metadata that its generator will throw if its called. As an example, suppose deprecated-generator was deprecated in 0.5.9.

(def-throwing-generator deprecated-generator
  "deprecated-generator has been deprecated. Use current-generator instead."
  {:deprecated "0.5.9"})

Then if the user looked up its documentation, they would see from cider:

clojure.test.check.generators/deprecated-generator
Deprecated in 0.5.9
  deprecated-generator has been deprecated. Use current-generator instead.

or if they used the generator and ran cider-test, they would see:

Test Summary
foobar.core-test

Ran 1 tests, in 1 test functions
1 errors


Results

Error in a-test
FIXME, I fail.
expected: (= "a" (sample deprecated-generator 5))
  actual: clojure.lang.ExceptionInfo: deprecated-generator has been deprecated. Use current-generator instead. {:deprecated "0.5.9"}





[TCHECK-40] Typo: alpha-numeric should be alphanumeric Created: 25/Aug/14  Updated: 02/Sep/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: John Walker Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File alphanumeric2.diff    
Patch: Code and Test

 Description   

This replaces all instances of alpha-numeric with alphanumeric. This includes renaming the following functions:

char-alpha-numeric -> char-alphanumeric
string-alpha-numeric -> string-alphanumeric



 Comments   
Comment by Reid Draper [ 26/Aug/14 2:22 PM ]

test.check hasn't reached 1.0, so we can break backward compatibility. However, I want to still communicate clearly to users. Maybe we can leave a definition of the hyphenated versions that raises a 'deprecated/removed' exception?

Comment by John Walker [ 27/Aug/14 10:44 AM ]

This seems reasonable. Are generators of this form alright?

(defn make-deprecated-generator
  ([msg map]
     (make-gen
      (fn [_ _]
        (throw (ex-info msg map)))))
  ([msg map cause]
     (make-gen
      (fn [_ _]
        (throw (ex-info msg map cause))))))

(def
  ^{:deprecated "TODO"}
  char-alpha-numeric
  "DEPRECATED - use char-alphanumeric instead."
  (make-deprecated-generator "DEPRECATED - use char-alphanumeric instead." {:deprecated "TODO"}))

If so, I can supply a macro to create these things. It would attach the metadata like docstrings and the deprecation version to the var, and usage might look like

(def-deprecated-generator char-alpha-numeric 
  "DEPRECATED - use char-alphanumeric instead."
  {:deprecated "TODO"})
Comment by Reid Draper [ 01/Sep/14 10:31 PM ]

I think I may be changing my mind here. I'm thinking we should have one release where the two names are both present. Make some noise about it in the release notes, and attach the :deprecated tag on the metadata. Would you mind making a patch to just add the new names, and add a ^{:deprecated ...} tag on the existing names? Sorry for the back and forth.

Comment by John Walker [ 02/Sep/14 2:11 PM ]

Thats fair. How should we handle it after that release?

Comment by John Walker [ 02/Sep/14 4:26 PM ]

This patch replaces alpha-numeric with alphanumeric, and attaches :deprecated "0.6.0" to char-alpha-numeric and string-alpha-numeric in favor of char-alphanumeric and string-alphanumeric.

Comment by Reid Draper [ 02/Sep/14 7:59 PM ]

Patch applied in 6def75ebc80cc5c1ab66ae956c76a7b402198852.





[TCHECK-39] such-that stops after 10 tries Created: 23/Aug/14  Updated: 26/Aug/14  Resolved: 26/Aug/14

Status: Resolved
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Paul Lange Assignee: Reid Draper
Resolution: Declined Votes: 0
Labels: None
Environment:

clojure 1.5.1, test.check 0.5.9



 Description   

The documentation for such-that states that "it will keep trying forever" till it finds values that satisfy f.

I use it with a prime? predicate to generate primes: (such-that prime? gen/pos-int)

This sometimes results in this error when running a tc/quick-check which contradicts the docs:
ExceptionInfo Couldn't satisfy such-that predicate after 10 tries. clojure.core/ex-info (core.clj:4327)



 Comments   
Comment by John Walker [ 25/Aug/14 5:27 PM ]

Where do you see this in the documentation?

https://github.com/clojure/test.check/blob/d6edf70cfa6cb20316f9377edf9630772ccef969/src/main/clojure/clojure/test/check/generators.clj#L268

"By default, `such-that` will try 10 times to generate a value that
satisfies the predicate. If no value passes this predicate after this number
of iterations, a runtime exception will be throw." [sic]

Comment by Paul Lange [ 26/Aug/14 2:52 AM ]

Thanks, saw it here (which seem out of date): https://clojure.github.io/test.check/clojure.test.check.generators.html#var-such-that

Comment by Reid Draper [ 26/Aug/14 2:19 PM ]

I've just updated that page. Apologies on it being out of date. Would be nice to have a little version selector thing on it, so you can know you're viewing docs for the version of test.check that you use.





[TCHECK-38] Generators for functions? Created: 15/Aug/14  Updated: 27/Aug/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Michael Sperber Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None


 Description   

test.check has generators for first-order types, but not for functions.

In the original QuickCheck, an additional type class ("Arbitrary") handles
functions.

If I implemented this and submitted a patch, would it be likely to be included?

Background: I wrote my own Clojure QuickCheck clone here:

https://bitbucket.org/sperber/deinprogramm.quickcheck

This has arbitraries and function generators, but not all of test.check's goodies.

I'm trying to decide whether to continue maintaining my QuickCheck clone or
instead to contribute ot test.check, so feedback would be much appreciated.



 Comments   
Comment by Reid Draper [ 21/Aug/14 2:16 PM ]

I'd love a patch for this! Would you like to write up a quick implementation proposal first, so we can iron out any architectural details before looking at the code?

Comment by Michael Sperber [ 25/Aug/14 2:06 AM ]

Would you be willing to look at my QuickCheck clone as a starting point? Its general implementation approach is very similar to yours, so I think you should feel right at home.

The caveat is that it introduces a type separate from generators - the "arbitrary", corresponding to the `Arbitray' type class in the Haskell code. Doing this to `test.check' would change the API, so maybe there, the `Generator' type should be extended by an optional `transformer' field.

Comment by Reid Draper [ 26/Aug/14 2:26 PM ]

Sure, I've already read through it a bit, but need to read in more detail. How does your implementation handle printing (showing) functions? Do functions shrink? Have you seen Showing and Shrinking Functions by Claessen ?

Comment by Michael Sperber [ 27/Aug/14 2:21 AM ]

Yes. Haven't done that yet, but it would be on my list. Given that functions are objects in Clojure, I think printing should be a little easier than in Haskell.





[TCHECK-37] Make random-number generation in test.check thread-safe Created: 15/Aug/14  Updated: 05/Sep/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Michael Sperber Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: enhancement

Attachments: File your-patch-file.diff    
Patch: Code and Test

 Description   

test.check uses the Java random-number generator, which is imperative.

This makes the testing non-deterministic in many settings.

It would be better if test.check used a purely functional rng, just
like the original Haskell version of QuickCheck.

I've attached a patch that does this. All but one of the original test
pass; I've modified the only failing test to not rely on an imperative
rng.

Note that it still has quick-check/make-rng supply a random seed.
IMHO, it should supply a fixed seed.



 Comments   
Comment by Reid Draper [ 15/Aug/14 5:45 PM ]

Thanks for the patch! I haven't had a chance to look it over in detail, but I do have a few introductory questions. You're correct that test.check currently uses a mutable RNG. While it may not be ideal, I haven't run into any user-facing cases where the use is non-deterministic. Do you have some particular example in mind where test.check is not deterministic now, given the same RNG seed? One place this could be an issue in the future is if we changed the way we walk the shrink tree, which would cause different values to be generated. I'd be keen on fixing this, but it's not an issue at the moment.

tl;dr: Under what circumstances, given the same seed, is test.check non-deterministic?

Comment by Michael Sperber [ 20/Aug/14 6:50 AM ]

Thanks for the prompt answer, and sorry for taking so long to reply.

The answer to your question is: None I'm currently encountering.

My motivation for the patch was a bit different, though:

1. I don't see an easy way to make things deterministic with defspec, running tests from, say, Leiningen.

2. While I can get reproducibility specifying the seed explicitly, this complicates the testing process, which should be as smooth as possible.

3. With the purely-functional rng, the generator monad is splittable: This enables me to write deterministic tests for concurrent programs.

#1 and #2 could also be addressed with the imperative RNG, not #3, though.

Comment by Reid Draper [ 21/Aug/14 2:15 PM ]

I don't see an easy way to make things deterministic with defspec, running tests from, say, Leiningen.

Indeed, this is simply an omission in the defspec macro. Calling the quick-check function directly allows you to pass in a seed. You can see the tests for this.

While I can get reproducibility specifying the seed explicitly, this complicates the testing process, which should be as smooth as possible.

This is how Haskell QuickCheck works too. If you don't specify a seed, tests are random. Would you prefer to get the same result, even without explicitly supplying a seed?

With the purely-functional rng, the generator monad is splittable: This enables me to write deterministic tests for concurrent programs.

You can write tests for concurrent programs now. The generated values are completely constructed before your code that may use threads is called. What am I missing?

Comment by Michael Sperber [ 25/Aug/14 2:02 AM ]

Ah, I didn't realize the shipped QuickCheck does this - sorry. But yes, I would greatly prefer to get the same result, unless explicitly instructed otherwise: This would solve the `defspec' problem.

Also, some test suites will pass or fail depending on the seed - particularly those involving floating-point may need a lot of test cases before they fail, and non-reproducibility might convince you that they pass - for a while.

You can write tests for concurrent programs now.

I was being less than clear: I meant concurrent tests for concurrent programs, where the concurrency is initiated by the test itself. In that case, you may want to call `quickcheck' from different threads, and would thus like to give each its own random-number generator.

Comment by Reid Draper [ 04/Sep/14 9:00 PM ]

Just noticed that newer Haskell QuickCheck uses this random number generator now .

Comment by Reid Draper [ 05/Sep/14 1:45 PM ]

Some more discussion of the problems with the proposed implementation:





[TCHECK-36] doc typo: 'excepts' should be 'accepts' Created: 05/Aug/14  Updated: 07/Aug/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Steve Miner Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File typo-excepts-should-be-accepts.patch    
Patch: Code

 Description   

typo in doc/intro.md



 Comments   
Comment by Reid Draper [ 07/Aug/14 11:51 AM ]

Thanks! Pushed in d2f46ad2fba3e7363cc0f22f40272088f2f01eb5.





[TCHECK-35] Improve symbol and keyword generators Created: 11/Jul/14  Updated: 23/Jul/14  Resolved: 23/Jul/14

Status: Resolved
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Eric Normand Assignee: Reid Draper
Resolution: Completed Votes: 0
Labels: enhancement, generator

Attachments: File namespaced-symbols-2014-07-11-14-38.diff     File namespaced-symbols-2014-07-23.diff    
Patch: Code

 Description   

Create a generator for symbols.

Also modify the keyword generator to allow :'s (as per the spec) (http://clojure.org/reader)

Create generators for namespaced symbols and keywords, as these were not tested before.

The new generators are added to gen/any.

A patch (namespaced-symbols-2014-07-11-14-38.diff) is attached.

Also a suggestion (but not in the patch): the edn-roundtrips test should be run separately on each type in gen/any to adequately search the space for errors.



 Comments   
Comment by Reid Draper [ 22/Jul/14 8:20 PM ]

Aside from +or--digit?, are all of the rest of the definitions in this patch intended to be public? It's a little unclear to me which of these are public generators and which are just helpers.

Comment by Eric Normand [ 23/Jul/14 2:06 PM ]

This patch is the same as the previous one with the non-interface functions set as private.

Patch: namespaced-symbols-2014-07-23.diff

Comment by Reid Draper [ 23/Jul/14 7:08 PM ]

Thanks! Merged in bc1650d4136d3453a6c16a363c1d5db36ad9a6b1.





[TCHECK-34] clojure.test reporting is uninformative Created: 21/Jun/14  Updated: 11/Aug/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Philip Potter Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None


 Description   

The clojure.test reporting results in output like:

FAIL in (always-fail) (clojure_test.clj:18)
expected: result
  actual: false

Note that the file and line number are always clojure_test.clj:18 and the expected value is always the literal string "result".

I'm not sure what the right output would be for expected/actual, but the incorrect file and line reporting means that clojure-test-mode consistently highlights the wrong line.



 Comments   
Comment by Philip Potter [ 21/Jun/14 9:10 AM ]

I think this is because the assertion is checked by calling clojure.test/is. clojure.test/is is being called for its return value, but it has the side effect of reporting failures. It's a macro, and it takes the literal symbol "result" as its expected expression, and takes the current file and line number to attach to the failure. It's really designed to be called by user code, not library code.

Comment by John Walker [ 11/Aug/14 3:55 PM ]

Well, one solution would involve taking advantage of the report functionality.

https://github.com/clojure/clojure/blob/master/src/clj/clojure/test.clj#L204
https://github.com/clojure/clojure/blob/master/src/clj/clojure/test.clj#L371





[TCHECK-33] test.check is entangled with clojure.test Created: 21/Jun/14  Updated: 21/Jun/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Philip Potter Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None


 Description   

test.check is tightly coupled to clojure.test; this means that it's difficult to use it with other test frameworks.

There is a dependency cycle between clojure.test.check and clojure.test.check.clojure-test.

quick-check uses clojure.test/report to print dots to the screen, meaning that quick-check necessarily depends on clojure.test



 Comments   
Comment by Philip Potter [ 21/Jun/14 8:54 AM ]

I wonder if the solution is to allow you to provide your own reporting functions to quick-check to report successes, failures, and trials? That way different frameworks can inject different behaviour as required.





[TCHECK-32] Default sizing on gen/any needs re-evaluation Created: 13/Jun/14  Updated: 04/Aug/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Luke VanderHart Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None


 Description   

The following innocuous-looking test blows the heap and then crashes a 4GB JVM with an out-of-memory error:

(tc/defspec merge-is-idempotent
  100
  (prop/for-all [m (gen/map gen/any gen/any)]
    (= m (merge m m))))

I understand how this happens, and how to fix it by adjusting the size parameters.

However, it would be great if using the defaults did not have the potential for such a nasty failure mode (particularly as the unwary user will have trouble determining that the fault is not in their code).



 Comments   
Comment by Philip Potter [ 21/Jun/14 8:29 AM ]

I was going to raise a separate ticket for this, but it seems like it might be related: gen/any (and any-printable) seem to take exponential time in the size of the input. See for example this session:

user> (time (dorun (gen/sample (gen/resize 50 gen/any-printable))))
"Elapsed time: 2204.284643 msecs"
nil
user> (time (dorun (gen/sample (gen/resize 55 gen/any-printable))))
"Elapsed time: 2620.717337 msecs"
nil
user> (time (dorun (gen/sample (gen/resize 60 gen/any-printable))))
"Elapsed time: 5923.636336 msecs"
nil
user> (time (dorun (gen/sample (gen/resize 65 gen/any-printable))))
"Elapsed time: 9035.762191 msecs"
nil
user> (time (dorun (gen/sample (gen/resize 70 gen/any-printable))))
"Elapsed time: 15393.687184 msecs"
nil
user> (time (dorun (gen/sample (gen/resize 75 gen/any-printable))))
"Elapsed time: 9510.571668 msecs"
nil
user> (time (dorun (gen/sample (gen/resize 80 gen/any-printable))))
"Elapsed time: 39591.543565 msecs"
nil

Apart from the anomaly at 75, adding 10 to the size seems to increase runtime by almost 3x.

Comment by Reid Draper [ 23/Jun/14 5:07 PM ]

I believe I should have a fix for this today, as well as an easier way to write custom, recursive generators.

Comment by Reid Draper [ 23/Jun/14 5:21 PM ]

This isn't fixing the OOM yet, but is making a big difference in making the size have a linear relationship with the number of elements generated, in a recursive generator. Here's branch: https://github.com/clojure/test.check/compare/feature;recursive-generator-helpers.

Comment by Reid Draper [ 23/Jun/14 7:16 PM ]

Fixed in https://github.com/clojure/test.check/commit/19ca756c95141af3fb9caa5e053b9d01120e5d7e. I'll try and get a snapshot build up tonight. Will be 0.5.9-SNAPSHOT. Check out the commit-message for a full explanation of how this now works.

Comment by Philip Potter [ 28/Jun/14 3:26 PM ]

awesome work! The new recursive-gen fn is great, too: I can immediately see a use case for generating arbitrary JSON-compatible data (ie vectors, maps, strings, numbers, bools, but no rationals, characters, symbols...).

Comment by Philip Potter [ 29/Jun/14 6:48 AM ]

Just re-tested on my machine; performance is vastly improved though still superlinear:

clojure.test.mode> (time (dorun (gen/sample (gen/resize 100 gen/any-printable))))
"Elapsed time: 101.907628 msecs"
nil
clojure.test.mode> (time (dorun (gen/sample (gen/resize 200 gen/any-printable))))
"Elapsed time: 302.341697 msecs"
nil
clojure.test.mode> (time (dorun (gen/sample (gen/resize 400 gen/any-printable))))
"Elapsed time: 1154.098163 msecs"
nil
clojure.test.mode> (time (dorun (gen/sample (gen/resize 800 gen/any-printable))))
"Elapsed time: 2954.889396 msecs"
nil
clojure.test.mode> (time (dorun (gen/sample (gen/resize 1600 gen/any-printable))))
"Elapsed time: 22335.200578 msecs"
nil

although since the default max-size is 200, this is very much no big deal.

Comment by Reid Draper [ 03/Jul/14 10:52 AM ]

I'm not too surprised that performance is still superlinear. What should be linear is the number of leaf nodes in the generated tree. We may eventually be able to do something a little more complex and have the total number of nodes be linear (including internal nodes). For now, however, it should be considered a bug if the relationship between leaf nodes doesn't grow linearly with the size parameter.

Comment by Reid Draper [ 04/Aug/14 8:35 PM ]

I'm still seeing that the original example is OOMing. I'm not sure the best way to solve that. For a single layer of nested generators, the above patch seems to be making a big difference. But when you make a map of gen/any, map doesn't know that it's arguments are themselves recursive generators. This means you might create 100 keys and values, each themselves, large recursive generators. There's a balance that has to be had between making the default recursive generators be 'large enough' by themselves, but not too large when used like this. Hmm.. I think I'll go ahead and release 0.5.9, and keep on thinking of ways to make this even better.





[TCHECK-31] Doc improvements: how to separate "size" from "range" Created: 11/Jun/14  Updated: 12/Jun/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Michael Nygard Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Size is discussed mainly in terms of recursive generators. However, it is crucial to understanding other aspects such as vector length and integer ranges.

Some examples like these would be helpful:

  • How to get a small number of samples with a large range.
  • How to get a large number of samples with a small range.


 Comments   
Comment by Reid Draper [ 12/Jun/14 9:13 AM ]

Agree. I'll try and get a patch up in the next couple of days. Do you just have something in mind going in the doc/ dir?





[TCHECK-30] Make Namespacing Consistent in generators.clj Docstrings Created: 31/May/14  Updated: 31/May/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Trivial
Reporter: Michael S. Daines Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File prefix.patch    
Patch: Code

 Description   

Three of the docs with examples in generators.clj aren't namespaced with "gen/", whereas everything else is. Add these in to make the documentation consistent and easy for people learning the library to cut and paste to test.






[TCHECK-29] Docstring typo Created: 31/May/14  Updated: 31/May/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Gary Fredericks Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File docstring-typo.patch    

 Description   

Patch attached.






[TCHECK-28] gen/elements should work with arbitrary collections Created: 23/May/14  Updated: 23/May/14  Resolved: 23/May/14

Status: Resolved
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Gary Fredericks Assignee: Reid Draper
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File TCHECK-28.patch    

 Description   

gen/elements currently fails for any collection on which nth fails.

We can fix this by calling vec on the input, which has the added benefit of ensuring that the subsequent indexed lookups (one per generated value) are constant time.

A patch to that effect is imminent.



 Comments   
Comment by Gary Fredericks [ 23/May/14 8:52 AM ]

Added TCHECK-28.patch.

Comment by Reid Draper [ 23/May/14 4:52 PM ]

Applied in 1df5a635bcf195c2f98717da3929f6c0d832ee0e.





[TCHECK-27] stateful PRNG + bind => nondeterministic shrink tree Created: 10/May/14  Updated: 11/May/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Gary Fredericks Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File TCHECK-27-p1.patch    

 Description   

bind inherently needs to consume randomness while shrinking, since the inner generator has to be run multiple times. Since the shrink tree is lazy and there's only one Random object providing the randomness, the resulting shrank values are sensitive to what order the tree is walked in.

I don't believe this affects normal usage, but there are various modifications I've made that I think are useful (e.g., resuming slow shrinks) that are thwarted by this nondeterminism.

Ideally we could stop using a stateful PRNG, but I think this would be a rather extreme change. A much smaller fix is to change bind to create a new Random object each time the inner function is called, based on a seed from the outer Random. I will have a patch that does this shortly, and it seems to fix the issues I've had.

Code for reproducing the issue:

(ns user
  (:require [clojure.test.check.generators :as gen]
            [clojure.test.check.rose-tree :as rose]))

(def the-gen
  (gen/bind gen/nat
            (fn [n]
              (gen/fmap (fn [m] [n m]) gen/nat))))

(defn reproduced?
  [seed]
  (let [make-rose-tree #(gen/call-gen the-gen (java.util.Random. seed) 100)
        rose-1 (make-rose-tree)
        rose-2 (doto (make-rose-tree)
                 ((fn [rt1]
                    ;; force the tree to be realized in a different way
                    (dorun (for [rt2 (rose/children rt1)
                                 rt3 (rose/children rt2)]
                             42)))))
        last-child #(-> % rose/children last rose/root)]
    ;; when these two are not equal, we've reproduced the problem
    (not= (last-child rose-1)
          (last-child rose-2))))

(frequencies (map reproduced? (range 10000)))
;; => {false 9952, true 48}


 Comments   
Comment by Gary Fredericks [ 10/May/14 2:16 PM ]

Attached TCHECK-27-p1, which makes the change I described in the description to the bind-helper function.

Comment by Gary Fredericks [ 11/May/14 8:27 AM ]

The only function that uses bind-helper is bind; however, bind is also implemented with gen-bind, and several other functions use gen-bind. So we'll have to check if gen-bind is sensitive to this as well (e.g., perhaps frequencies has this issue?). If so, I'm not sure what that means about the best way to fix it. Maybe there's an alternative fix that could happen in gen-bind, or maybe the other functions could be rewritten with bind?





[TCHECK-26] defspec does not respect (is (thrown? ...)) Created: 20/Apr/14  Updated: 13/May/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Sung Pae Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None


 Description   

The following test does not pass:

(quick-check 1
  (for-all [x g/any]
    (is (thrown? Throwable (assert false)))))

This can be worked around by wrapping the (is ...) form with a call to boolean:

(quick-check 1
  (for-all [x g/any]
    (boolean (is (thrown? Throwable (assert false))))))

The (is (thrown? ...)) form does not return true|false, but will return the Exception itself when it passes.

Thank you!



 Comments   
Comment by Reid Draper [ 13/May/14 6:14 PM ]

This is correct. And I'm not yet sure what the best way forward is, but my hunch is that we'll need to support non-boolean return values from tests. Maybe something that implements a ->pass? function.





[TCHECK-25] shuffle generator Created: 17/Apr/14  Updated: 26/May/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Gunnar Völkel Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Adding a generator that shuffles the output of a given collection generator would be really useful.
Short example:

(gen/sample (gen/shuffle (gen/vector get/nat))) ;=> ([5 2 4] [9 1 7 8] ...)

This could be used to add a generator for permutations of integers 1,2,...,n which I also missed several times already.



 Comments   
Comment by Reid Draper [ 13/May/14 6:17 PM ]

Yeah this would be cool. Should it shrink toward the original collection-ordering?

Comment by Gunnar Völkel [ 26/May/14 10:55 AM ]

Yes, that was the idea when we talked about that topic on #clojure. Otherwise, you can just use gen/int to create a seed and use java.util.Random together with java.util.Collections/shuffle.





[TCHECK-24] Add a retry-limited version of `such-that` Created: 17/Apr/14  Updated: 13/May/14  Resolved: 13/May/14

Status: Resolved
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Jean Niklas L'orange Assignee: Reid Draper
Resolution: Completed Votes: 0
Labels: None


 Description   

The docstring of such-that states that

Eventually we will add another generator combinator that only tries N times before giving up.

So I found it reasonable to add an issue just so it's not forgotten. Seems reasonable to have it throw a RuntimeException with an explanation that such-that attempted to do more than N tries.

Additionally, I think it would be valuable to have a runtime-error in the general such-that implementation, e.g. on 10000 retries, possibly with an explanation on how to override the default retry-limit. I've heard of many people (myself included) not being really sure where their infinite loop is within their test.check code, and I guess such-that is the culprit in very many cases. This might help debugging those issues, although it would break backwards compatibility.



 Comments   
Comment by Reid Draper [ 13/May/14 6:12 PM ]

Fixed in 719d64d60419638f22e3ceca5e37fe075df652ea. Should be in the next release.





[TCHECK-23] Clojure Keywords must begin with a non-digit. Created: 15/Apr/14  Updated: 16/Apr/14  Resolved: 16/Apr/14

Status: Resolved
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Eric Normand Assignee: Reid Draper
Resolution: Completed Votes: 0
Labels: None

Attachments: File better-keywords.diff    

 Description   

Per http://clojure.org/reader, Clojure Keywords must begin with a non-numeric character. Currently, test.check generates keywords from strings of alpha-numeric characters, and sometimes the first character is a digit. Although the current reader on the JVM allows it, the ClojureScript reader fails with keywords like :1, :2aa, etc.

Here is the current code:

(def keyword
"Generate keywords."
(->> string-alpha-numeric
(such-that #(not= "" %))
(fmap core/keyword)))

Proposed fix:

(def keyword
"Generate keywords."
(->> (tuple char-alpha string-alpha-numeric)
(fmap #(apply str %))))
(fmap core/keyword)))

char-alpha will need to be defined.

This would also alleviate the need for the such-that to filter out empty strings.



 Comments   
Comment by Reid Draper [ 15/Apr/14 8:03 PM ]

+1. Do you have a CA signed, Eric? And would you like to provide a patch? If not, I'd be happy to fix this.

Comment by Eric Normand [ 16/Apr/14 6:35 AM ]

My CA is filed with Cognitect.

I'll get a patch in today.

Comment by Eric Normand [ 16/Apr/14 9:21 AM ]

better-keywords.diff contains a patch to conform to the reader specs. All tests pass.

Comment by Reid Draper [ 16/Apr/14 10:22 PM ]

Merged in 4528b5ed391d6127b79f4db6bc5e086613da0d81

Comment by Reid Draper [ 16/Apr/14 10:22 PM ]

Thanks Eric!





[TCHECK-22] Move rose-tree code into a separate namespace. Created: 13/Apr/14  Updated: 13/Apr/14  Resolved: 13/Apr/14

Status: Resolved
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Gary Fredericks Assignee: Reid Draper
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File rose-namespace.patch    

 Description   

The rose tree code is logically separate from the rest of the generator code, and is already pseudo-namespaced by the inclusion of the word "rose" in the function names.



 Comments   
Comment by Gary Fredericks [ 13/Apr/14 2:35 PM ]

Attached a patch that moves the rose-tree functions into their own namespace, and removes "rose" from the function names.

The latter part is a bit trickier, and a couple times involved naming collisions for t he names root and children which are now functions in the rose-tree namespace but are also often locals in that namespaces as well. I twice changed the locals to the-root or the-namespace to resolve the collision.

Comment by Reid Draper [ 13/Apr/14 4:58 PM ]

Merged in 9cc5cac08a9dc5ec0d327e12c502dea1f7741958. Thanks!





[TCHECK-21] Rerunning a particular failure is difficult. Created: 11/Apr/14  Updated: 12/Apr/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Gary Fredericks Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Test.check has the concept of a seed that is used when a property is tested. The seed can be supplied or generated by the current time. Either way, the single seed is used to generate all the trials for the test. If a test fails, the only way to try to re-test the property on the same inputs is to run all of the tests again using the same seed. If the tests were running for hours before a failure was found, this is prohibitive.

I would like instead to be able to check the property for exactly the input that failed. One step in that direction is to have an explicit different seed for each trial (perhaps generated from an initial metaseed), and on failure to report a [seed size] pair that can be used to run a test a single time.

The way this interacts with shrinking is interesting though, since it's just as likely that I'll want to re-run on the shrunk value from a failure. Since the shrunk value was never explicitly generated from a known [seed size], just that information is insufficient. We could perhaps report [seed size shrink-path] where the third element is a list of indexes to walk through the shrink tree with. Probably the worst part about that is it might be rather long.

In any case I think something like this would be more useful than the current setup. I'll probably be hacking up something similar for a fork branch I'm maintaining, but if we can settle on a specific design I'd be happy to cook up a patch.



 Comments   
Comment by Reid Draper [ 12/Apr/14 10:28 AM ]

When a test fails, we do return the arguments that originally failed, and the arguments of the shrunk test. Is this insufficient?

For example:

{:result false,
       :failing-size 45,
       :num-tests 46,
       :fail [[10 1 28 40 11 -33 42 -42 39 -13 13 -44 -36 11 27 -42 4 21 -39]],
       :shrunk {:total-nodes-visited 38,
                :depth 18,
                :result false,
                :smallest [[42]]}}

see :fail and [:shrunk :smallest].

Comment by Gary Fredericks [ 12/Apr/14 11:00 AM ]

It's certainly adequate to reproduce in theory, but I don't know of any built in mechanism that makes this easy. Here's what I end up doing:

Given this:

(defspec foo
  (prop/for-all [x gen]
    (f x)))

and after getting a failure with a 50-line shrunk value (e.g. :bar), I manually pretty-print it and paste it back into my file like so:

(def shrank
  ':bar)

(defspec foo
  (prop/for-all [x #_gen (gen/return shrank)]
    (f x)))

And now I can run (foo 1) to re-run with the failing value.

This awkwardness is partly due to three facts:

  1. My generators create large unwieldy values
  2. My property is a large body of code rather than a single function
  3. There's no easy way to test a property on a specific value

I could mitigate some of the pain by fixing #2 – having each one of my tests split into a defspec / prop/for-all and a plain function. But #1 is not easy to fix, and having a small tuple is rather nicer for me.

I've already written a POC for this and it's working rather well. I used the term :key to refer to the tuple, so the term doesn't conflict with :seed. I end up calling the test like (foo 0 :key [329489249329323 19 []]), but I'll probably think up something else that doesn't require passing a dummy first arg.





[TCHECK-20] Code cleanup Created: 11/Apr/14  Updated: 12/Apr/14  Resolved: 12/Apr/14

Status: Resolved
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Trivial
Reporter: Gary Fredericks Assignee: Reid Draper
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cleanup.patch     Text File core-slash.patch     Text File flatten.patch    

 Description   

I've been doing some hacking on test.check, so just trying to leave things better than I found them.



 Comments   
Comment by Gary Fredericks [ 11/Apr/14 8:53 PM ]

Attached another patch that changes clojure.core/ to core/ in the generators namespace, in case you like that sort of thing.

Comment by Gary Fredericks [ 12/Apr/14 6:53 AM ]

Attached a third patch for removing a use of flatten.

Comment by Reid Draper [ 12/Apr/14 6:16 PM ]

Thanks Gary. I've merged all three patches in 63bc79fdded00211f4b9a51561920965d4e2c67d.





[TCHECK-19] Permit a data-structure containing generators to be used as a generator Created: 11/Apr/14  Updated: 22/Jun/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Darrick Wiebe Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File add_record.diff    

 Description   

I'm working through some of the examples in John Hughes' "QuickCheck Testing for Fun and Profit" paper. I'd like the structures I generate in those tests to look like the this:

[:apply `reg [(gen-name) (gen/elements pids)]]

Rather than the way they must be expressed now:

(gen/tuple (gen/return :apply) (gen/return `reg) (gen/tuple (gen-name) (gen/elements pids)))

I think a simple recursive function could be used to generate these, with the caveat that currently generators create a map {:gen #<fn...>}, which I'd propose to change to a record so that we could distinguish between a generator and a map data-structure.

This seems to be implemented to good effect in Quvig QuickCheck already:

In general, Quviq QuickCheck permits any data-structure containing embedded generators to be used as a generator for data-structures of that shape—something which is very convenient for users, but quite impossible in Haskell, where embedding a generator in a data-structure would normally result in a type error. This technique is used throughout the generators written at Ericsson. – Hughes



 Comments   
Comment by Darrick Wiebe [ 11/Apr/14 5:06 PM ]

Playing around in the REPL today, I came up with this code that seems to work well converting arbitrary literals into generators:

(defprotocol LiteralGenerator
  (-literal->generator [literal]))

(defn literal->generator [literal]
  (cond
    (satisfies? LiteralGenerator literal) (-literal->generator literal)
    (vector? literal) (apply gen/tuple (mapv literal->generator literal))
    (and (map? literal) (= [:gen] (keys literal)) (fn? (:gen literal))) literal
    (map? literal) (gen/fmap (partial into {}) (literal->generator (mapv vec literal)))
    (set? literal) (gen/fmap set (literal->generator (vec literal)))
    (list? literal) (gen/fmap (partial apply list) (literal->generator (vec literal)))
    :else (gen/return literal)))

Generating from a record is probably something that would be generally useful, so I added this as well:

(defn record->generator
  ([record]
   ; Is there a better way to do this?
   (let [ctor (eval (read-string (str "map->" (last (clojure.string/split (str (class record)) #"\.")))))]
     (record->generator ctor record)))
  ([ctor record]
   (gen/fmap ctor (literal->generator (into {} record)))))

Which enables me to extend a record like this:

(defrecord Foo [a b]
  LiteralGenerator
  (-literal->generator [this] (record->generator map->AbcDef this)))

I haven't looked at the possibility of baking this code into test.check at all yet. I'd like feedback on what I've got so far before pursuing this any further.

Comment by Reid Draper [ 11/Apr/14 5:31 PM ]

So, I have kind of mixed feelings about this. I agree it's convenient, but I also worry that being loose like that can allow more accidents to occur, and doesn't force users to make the distinction between values and generators. For example, what if you do something like this: [int int]. Did you mean to type [gen/int gen/int], or do you really intend to have written the equivalent of [(gen/return int) (gen/return int)]? If every function that expects a generator can also take a value, we can no longer start adding error-checking to make sure you're correctly passing generators. On that same token, I do see the benefit of being able to use data-structure literals. What if we wrote a function that you had to use to create a generator with this syntax, something like: {{(gen/literal [:age gen/int])}}. That way you could opt-in to this syntax, but only within the scope of gen/literal?

Comment by Darrick Wiebe [ 11/Apr/14 5:40 PM ]

I agree that is a concern, and since you're not guaranteed to see generator output, it might be better to be explicit about using gen/literal. It's still a nice clean solution. Fortunately, that's exactly what I've written! We'd just need to rename literal->generator to literal and install it into the clojure.test.check.generators namespace.

Comment by Reid Draper [ 11/Apr/14 6:56 PM ]

I think the first step is changing generators to use Records. Interested in making a patch for that?

Comment by Darrick Wiebe [ 11/Apr/14 8:31 PM ]

A very simple patch to use a Generator record rather than a simple {:gen ƒ) map.

Comment by Reid Draper [ 12/Apr/14 6:15 PM ]

I've applied the record-patch in ef132b5f85a07879f01417c9104aa6dea771fdb4. Thanks. I've also added a generator? helper function.

Comment by Philip Potter [ 22/Jun/14 4:39 PM ]

I spotted something which seemed relevant: ztellman/collection-check defines a tuple* fn which is like gen/tuple but automatically wraps literals with gen/return.

Notably, despite not solving the general case, this would have solved the example in the issue description.





[TCHECK-18] Output of failed defspec should result in copy and paste-able data. Created: 08/Apr/14  Updated: 08/Apr/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Jason Gilman Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None


 Description   

During a failure of a defspec the generated example data that caused the failure is not output in a way that makes it easy to copy and paste for testing. For example if the failed data was {:a "foo" :b "bar"} it will be displayed in the REPL as {:a foo, :b bar}. This is happening because the result of testing is printed by clojure.test.check.clojure-test/assert-check with println:

(defn- assert-check
[{:keys [result] :as m}]
(println m)
(if (instance? Throwable result)
(throw result)
(ct/is result)))

If it was changed to use pr-str then the output would be copy and paste-able:

(defn- assert-check
[{:keys [result] :as m}]
(println (pr-str m))
(if (instance? Throwable result)
(throw result)
(ct/is result)))

The following example spec output is better when I manipulate a local copy of test.check.

(defspec test-output 1
(for-all [n (gen/return {:a "foo"})]
(= n {:b "foo"})))

Output without change:
{:test-var test-output, :result false, :failing-size 0, :num-tests 1, :fail [{:a foo}], :shrunk {:total-nodes-visited 0, :depth 0, :result false, :smallest [{:a foo}]}}

Output with change:
{:test-var "test-output", :result false, :failing-size 0, :num-tests 1, :fail [{:a "foo"}], :shrunk {:total-nodes-visited 0, :depth 0, :result false, :smallest [{:a "foo"}]}}



 Comments   
Comment by Reid Draper [ 08/Apr/14 9:20 PM ]

Agree. This should get fixed.





[TCHECK-17] shrinking/test running treat ctrl-c (InterruptedException) as Created: 04/Apr/14  Updated: 06/Apr/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Tom Crayford Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: None


 Description   

If you're testing a long running or blocking test (for example, I've been
working on finding race conditions with state machines ala John Hughes' keynote
at clojure west), it's very frustrating to not be able to ctrl-c tests when
they're blocking/stalled/slow. This is because the shrinking/running processes
are naive, and only catch Exception, without special casing
InterruptedException.

I'd very much like to see this fixed - it's a blocker on me actually using
test.check on my project, and very definitely a blocker on the race
condition/state machine work I've been doing. I've had a couple of runs at it,
but got pretty confused at aborting the shrink correctly (likely that's me
being dumb though).

To reproduce:

(tc/quick-check (prop/for-all [a (gen/any)] (Thread/sleep 10000)))

I'd expect to see:

the test run finishes

What happens instead:

test.check treats the InterruptedException as a failure and continues shrinking
as though that run had failed.



 Comments   
Comment by Reid Draper [ 06/Apr/14 11:44 AM ]

Agree. I have some ideas about what the best longterm solution is here, but am still a bit fuzzy. That being said, I think there are some simple incremental solutions that can help people now. I'll take a stab at something this week and gather some feedback.





[TCHECK-16] docstrings/comment cleanup Created: 03/Apr/14  Updated: 06/Apr/14  Resolved: 06/Apr/14

Status: Resolved
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Trivial
Reporter: Gary Fredericks Assignee: Reid Draper
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File vocabularic-pedantry.patch    

 Description   

Deleted a few apostrophes.



 Comments   
Comment by Reid Draper [ 06/Apr/14 10:53 AM ]

Applied in 2b2e5fac123e798a88a01e536bd12219395b09e2.





[TCHECK-15] gen/for macro for alternate combinator syntax Created: 02/Apr/14  Updated: 23/May/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Gary Fredericks Assignee: Reid Draper
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File TCHECK-15-p1.patch     Text File TCHECK-15-p2.patch     Text File TCHECK-15-p3.patch     Text File TCHECK-15-p4.patch     Text File TCHECK-15.patch    

 Description   

I think the syntax of clojure.core/for would be a good fit for test.check's combinators. For example:

(defn gen-even-subset
  "Returns a generator that generates an even-cardinality
   subset of the given elements"
  [elements]
  (gen/for [bools (apply gen/tuple (repeat (count elements) gen/boolean))
            :let [true-count (->> bools (filter identity) (count))]
            :when (even? true-count)]
    (->> (map list bools elements)
         (filter first)
         (map second)
         (set))))

This combines the capabilities of fmap, bind, and such-that into a familiar syntax.

One downside here is the temptation to use multiple clauses for independent generators, resulting in a use of gen/bind when gen/tuple would be simpler and presumably shrink easier. An approach to this is an additional supported clause, perhaps called :parallel, that uses the syntax of :let to provide the functionality of gen/tuple:

(gen/for [:parallel [n1 gen/nat
                     n2 gen/nat]
          :let [sum (+ n1 n2)]]
  {:nums [n1 n2] :sum sum})

Compared to gen/tuple, this has the advantage of placing generators syntactically next to names, rather than segregating the generators from the names.

The :parallel feature has not been added to the current patches.



 Comments   
Comment by Gary Fredericks [ 05/Apr/14 3:23 PM ]

I think there might be some design ambiguity around the meaning of :when. In particular, in the following contrived example:

(for [n nat
      v (vec (return n))
      :let [sum (reduce + v)]
      :when (pos? sum)]
  v)

In my default design this can hang, for the same reason that this code can hang:

(bind nat 
      (fn [n]
        (such-that
          (fn [v] (pos? (reduce + v)))
          (vector (return n)))))

But it could just as well have been written:

(such-that 
  (fn [v] (pos? (reduce + v)))
  (bind nat (fn [n] (vector (return n)))))

So the issue is whether a :when filter is applied to just the previous generator or to all of the previous generators. I have some hazy notion that the latter would be less efficient in some cases, but I'm not sure what. So I think our options are:

  1. Decide to always do it one way or the other
  2. Provide a third keyword (:when-all?) with different behavior
  3. Don't write this macro at all because it's too difficult to understand

My gut is to do option 1 and just apply :when to the previous generator.

Comment by Gary Fredericks [ 08/Apr/14 9:24 PM ]

Attached my initial draft. The implementation took a lot more thought than I expected, and is a bit subtle, so I included some inline comments explaining the structure of the macro.

Comment by Gary Fredericks [ 13/Apr/14 8:33 PM ]

Attached TCHECK-15-p1.patch, updated to apply to the current master.

Comment by Gary Fredericks [ 16/Apr/14 9:51 PM ]

Attached TCHECK-15-p2.patch which adds a note to the docstring about independent clauses, shrinking, and tuple.

Comment by Gary Fredericks [ 16/Apr/14 9:58 PM ]

Attached TCHECK-15-p3.patch which fixes one bug and one redundancy in namespace aliases.

Comment by Gary Fredericks [ 13/May/14 10:37 AM ]

Attached TCHECK-15-p4.patch which fixes a bug with destructuring (and adds a regression test).

Comment by Gary Fredericks [ 13/May/14 10:38 AM ]

Also might be helpful to note that I've put this in my test.check utility library for now: https://github.com/fredericksgary/test.chuck#for.





[TCHECK-14] re-organize README and doc/ Created: 29/Mar/14  Updated: 29/Mar/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Reid Draper Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: documentation


 Description   

I'd like to re-organize the README, and focus on making it easier for users to find the information they need. The README is currently pretty long, and could be information overload. My current thinking is that the new README should be short, and focus on providing links to people based on their experience with test.check. I think something like the following three headings would be useful:

1. First time user. Show me what this is all about.

  • Focus on 'seeing something cool' within five minutes. Maybe include some example tests in examples/. This way new users can simply clone the repository and run a test in under two minutes.

2. Still getting started, show me some tutorials and examples.

  • This will probably build on the existing doc/intro.md file, which currently describes how to write generators.

3. Existing user, show me API documentation, release notes, advanced techniques.

  • API documentation
  • Release-notes, make it clear these are up-to-date, and have clear instructions when there are breaking changes
  • Advanced techniques/tutorials





[TCHECK-13] Add generated API documentation for test.check Created: 27/Mar/14  Updated: 29/Mar/14  Resolved: 29/Mar/14

Status: Resolved
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Jason Gilman Assignee: Reid Draper
Resolution: Completed Votes: 0
Labels: None


 Description   

The simple-check README had a link to API documentation (http://reiddraper.github.io/simple-check) that was very useful. This appears to be missing for test.check. Please add generated documentation and a link to it on the test.check README.



 Comments   
Comment by Reid Draper [ 29/Mar/14 5:47 PM ]

Pushed: http://clojure.github.io/test.check/

Comment by Nicola Mometto [ 29/Mar/14 5:54 PM ]

Reid, clojure contrib repositories have the webdoc managed by autodoc so that they are shown in http://clojure.github.io/

I suggest you ping Tom Faulhaber so that he can set up the autodoc page for test.check, he will also add a hook so that the autodoc page will be updated automatically at every change





[TCHECK-12] Random-number seed isn't returned on test failure Created: 24/Mar/14  Updated: 25/Mar/14  Resolved: 25/Mar/14

Status: Resolved
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Reid Draper Assignee: Reid Draper
Resolution: Completed Votes: 0
Labels: None


 Description   

The random seed used to run the test is returned on test success, but not failure. We should return it on failure too.



 Comments   
Comment by Reid Draper [ 25/Mar/14 11:33 AM ]

Fixed in https://github.com/clojure/test.check/commit/9ba775a9fd69591ef065051be6c2b727387daff4.





[TCHECK-11] clojure.test.check.generators/rand-range includes upper bound Created: 14/Mar/14  Updated: 15/Apr/14  Resolved: 15/Apr/14

Status: Resolved
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Steve Kim Assignee: Reid Draper
Resolution: Completed Votes: 1
Labels: None

Attachments: File private-rand-range.diff    

 Description   

While playing with the rand-range function in the clojure.test.check.generators namespace, I was surprised that

(range-range lower upper)

can generate integers equal to the upper bound. This behavior is inconsistent with the zero-based numbering convention that ranges of integers are lower-inclusive, upper-exclusive.

I know that this function appears in a section that is commented

;; Internal Helpers

but it isn't a private function, and it is useful for users who need to implement custom generators. Can it be changed? It looks like the choose function in the same namespace is the only other function that would need to be changed too, along with unit tests.

If others agree that this is a bug that ought to be fixed, I volunteer to submit a patch, after I provide my signed Contributor Agreement.



 Comments   
Comment by Steve Kim [ 14/Mar/14 2:04 PM ]

Sorry for the typo. I meant

(rand-range rnd lower upper)
Comment by Reid Draper [ 16/Mar/14 8:06 PM ]

Both Erlang and Haskell QuickCheck do this too. I'll admit I mostly just followed in their steps, and that's it's a bit unexpected, compared to Clojure functions like 'range'. However, without being inclusive on the top-end, you wouldn't be able to create generators that fill an entire 'type range'. For example: (gen/sample (gen/choose Integer/MIN_VALUE Integer/MAX_VALUE)). Thoughts?

Comment by Chas Emerick [ 17/Mar/14 4:06 AM ]

I agree that it's surprising at first, but the utility is undeniable (compared to e.g. range as mentioned, the behaviour of which you need to compensate for in various ways in order to include upper bounds).

Comment by Jean Niklas L'orange [ 17/Mar/14 6:11 AM ]

On the visibility of this function: As far as I can see, rand-range is just used once in generators, and not used anywhere else. Whereas it is a useful function in general, I don't think it is that useful for people who need to implement custom generators (You can bind/fmap over choose instead).

I assume it wouldn't be a problem to make this private.

Comment by Steve Kim [ 17/Mar/14 10:12 AM ]

I did not know the QuickCheck convention, and I had not considered the need to fill an entire type range like

(gen/sample (gen/choose Integer/MIN_VALUE Integer/MAX_VALUE))
. Thanks for the explanation!

The docstring for choose states that it is inclusive, whereas rand-range is not documented. I think that new users would be less confused if either (1) rand-range had a docstring, or (2) rand-range were made private, because it only makes sense as a helper to implement the choose function. This is a trivial nitpick; I won't complain if this issue is resolved as Won't fix.

Comment by Reid Draper [ 17/Mar/14 10:20 AM ]

Sounding like we're all in favor of making range-range private. I'm happy to take a patch if someone wants to contribute. Otherwise I'll fix it today or tomorrow.

Comment by Jean Niklas L'orange [ 15/Apr/14 5:49 AM ]

I have attached the straightforward patch to this issue.

There is not much to elaborate on, only that the test suite now refers to the var containing the function, not the function itself (as it now is private).

Comment by Reid Draper [ 15/Apr/14 7:58 PM ]

Fixed in 364710da692e66fca7310bade4388cf9439e3ef1, thanks!

Comment by Reid Draper [ 15/Apr/14 7:58 PM ]

364710da692e66fca7310bade4388cf9439e3ef1





[TCHECK-10] defspec makes it impossible to specify :max-size (and seed) Created: 04/Mar/14  Updated: 06/Sep/14  Resolved: 06/Sep/14

Status: Resolved
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Jean Niklas L'orange Assignee: Reid Draper
Resolution: Completed Votes: 0
Labels: None

Attachments: File defspec.diff    

 Description   

While defspec is very valuable for integration with clojure.test, it is almost impossible to specify :max-size for the quick-check call. The only way I've found around it right now is by using the following macro:

(defmacro ^:private change-max-size
  [name max-size]
  `(alter-meta! (var ~name) assoc
                :test (fn [] (#'ct/assert-check
                             (assoc (~name ct/*default-test-count*
                                           :max-size ~max-size)
                               :test-var (str '~name))))))

This macro alters the meta of a named test such that the quick-check-call uses a specified expression for max-size, but obviously this is a very hacky solution. Preferably, I would be able to specify the :max-size value directly through defspec. I imagine that something like this could work:

(defspec my-test
  {:max-size 40, :num-tests 400}
  (my-test-here))

Essentially, if the optional argument (currently named default-times) is a map, inspect the map for values given, or use defaults if not.



 Comments   
Comment by Reid Draper [ 04/Mar/14 8:52 PM ]

I like the idea of making the first (optional) argument a map. This allows us to add in more options in the future, without breaking existing code.

Comment by John Walker [ 03/Sep/14 10:06 AM ]

Do we want to maintain backwards compatibility with

(defspec my-test number-of-times (my-test-here))

?

Comment by Reid Draper [ 03/Sep/14 6:02 PM ]

I spoke with Gary Fredericks about this earlier today, we're both thinking it seems reasonable to keep the current Int as a value, but also accept a map. The map will contain the same optional keys as the quick-check function, with an additional num-tests key. That seem reasonable?

Comment by John Walker [ 03/Sep/14 8:34 PM ]

Yes, I think that is the right approach. I'll get a first draft in for us tonight.

Comment by Reid Draper [ 03/Sep/14 8:54 PM ]

Great, thanks! As an aside, I've noticed that quick-check takes var-args, and not a last-argument-map. I'd probably like to change this as well, but am not sure how to keep backward compatibility there...

Comment by John Walker [ 03/Sep/14 9:14 PM ]

This patch adds a map as an alternative for defspec's options. Users can specify max-size, num-tests and seed using it. It also changes the way in which the function arguments produced by defspec are qualified. Example: Before it was max-size_auto_1448, now it is max-size.

Comment by John Walker [ 03/Sep/14 9:18 PM ]

Great, thanks!

You're welcome! Thanks for test.check.

As an aside, I've noticed that quick-check takes var-args, and not a last-argument-map. I'd probably like to change this as well, but am not sure how to keep backward compatibility there...

Probably the best way to maintain backward compatibility here would be to do some parsing on the varargs ourselves. It would require a check to see if the input is a map, etc. It's not a lot of trouble, but it's definitely a compromise.

Comment by Reid Draper [ 06/Sep/14 4:51 PM ]

Merged, with some white-space cleanup and added tests in c3d6134a5eef3a3c45369cc3f3f9416d23d30f61. Thanks!





[TCHECK-9] Properly document how :max-size works Created: 04/Mar/14  Updated: 14/Apr/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Jean Niklas L'orange Assignee: Reid Draper
Resolution: Unresolved Votes: 0
Labels: documentation


 Description   

I have had several issues where I thought increasing quick-check's num-tests argument would increase the time taken linearly, but surprisingly it grew quadratically. This is related to the auto-increasing of the size to call-gen, which I believe it isn't documented well enough. It also seems like other people have had trouble with this functionality, see the reply to announcement of clojure.test.check.

Preferably, it should be mentioned in the docstring and the tutorial that the size of the structures generated grows linearly with the amount of tests you have, up to 200. As a result, doubling the amount of tests does not necessarily double the amount of time taken to run the tests.

In some of my tests, this behaviour increases the running time exponentially, and I actually used quite some time figuring out why. The documentation on recursive generators decreases the size of the binary tree in half, but it is not explained why it is split in half. While obvious in hindsight, explicitly explaining that recursive structures should have an amount of nodes proportional to the input size argument and how it is done, would be beneficial.



 Comments   
Comment by Reid Draper [ 04/Mar/14 8:50 PM ]

+1. This is not well-documented now. Further, I'm still not 100% sure that halving inside of a recursive generator is the right thing to do. For some structures, log2 may be more appropriate. In the mailing list post you mention, gen/any is clearly not bounded enough at roughly size=90, consider it's using more than 1GB of heap.

Comment by Reid Draper [ 14/Apr/14 8:58 PM ]

I've made things a little better in 57df9b9b8f2fe61a798fbf683046e8d1aa128228.





[TCHECK-8] Bound tests and shrinks in time Created: 04/Mar/14  Updated: 04/Mar/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Jean Niklas L'orange Assignee: Reid Draper
Resolution: Unresolved Votes: 1
Labels: None


 Description   

It should be possible to bound tests and shrinks in time.

The original issue also mentions shrink interruption through Ctrl-c, but this is already covered in #TCHECK-4.

Original issue - #9 GitHub






[TCHECK-7] Add nested property support Created: 04/Mar/14  Updated: 26/May/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Jean Niklas L'orange Assignee: Reid Draper
Resolution: Unresolved Votes: 2
Labels: None


 Description   

Including functionality to add support for nested properties would be beneficial. An example of how this may work follows:

(defn exists?
  [e coll]
  (some #{e} coll))

(defn remove-elem
  [e coll]
  (remove #{e} coll))

;; this test should fail, and illustrates nested properties
(quick-check (for-all [v (gen/vector gen/int)]
               (for-all [e (elements v)]
                 #(exists? e (remove-elem e v)))))

The requirement for this is support for reifying properties, which is not currently implemented.

Original issue - GitHub #10



 Comments   
Comment by Maciej Jaśkowski [ 14/May/14 4:14 PM ]

What's the status of this issue?
It would help us a lot to have this functionality up and running in the following case: I want to verify my path finding algorithm finds optimal solution in the given graph G, so I generate the graph and a big number of random paths and check that all the random paths are "less optimal" then the one found by the algorithm.

Having nested for-all that would be simple. Is there a way around?

Comment by Reid Draper [ 14/May/14 4:24 PM ]

Sorry for the lack of update here. Nested properties are still in a bit of hammock-time on my part. The good news is they are merely convenient, and you can achieve the exact same effect with clojure.test.check.generators/bind. You can see an example of bind being used here.

Comment by Maciej Jaśkowski [ 15/May/14 12:08 AM ]

Apparently I need some more hints because all I can generate (using gen/bing, btw) are pairs '(graph list-of-random-paths)

That is useful as long as the algorithm works as expected. However, once it fails it's pretty hard to figure out which of random-paths failed it.

Comment by Reid Draper [ 15/May/14 1:02 PM ]

Can you show me how you would write the test with nested properties, and then perhaps I can help you rewrite it to use bind instead?

Comment by Maciej Jaśkowski [ 15/May/14 3:58 PM ]

Great! Thank you!

Please, find the most important part below.

(defn gen-matrix-and-paths [size]
    (gen/bind
      (gen-matrix gen/s-pos-int size)
      (fn [matrix]
        (let [ nodes (into [] (range (count matrix)))
               gen-perms (gen-permutations nodes)
               perms (distinct (gen/sample gen-perms 500)) ]
          (gen/tuple 
            (gen/return matrix) 
            (gen/return perms))))))

gen-matrix generates a square matrix of incidence e.g. [[0 1] [4 0]]
gen-permutations generates a permutation of provided vector

Comment by Reid Draper [ 15/May/14 4:41 PM ]

I'm confused. What issue are you running into? The code you've pasted uses bind, and not a nested property.

Comment by Maciej Jaśkowski [ 17/May/14 3:26 AM ]

Ok but now I can only write something like this:

(prop/for-all [data (gen-matrix-and-paths 6)]
   (let [ [matrix random-paths] data ]
     (not-worse-then-others? tsp matrix random-paths))))

which, if my 'tsp' algorithm is not working correctly would point me to a matrix and a vector (length 500) of paths one of which is counterexample. It would be better if instead I got the matrix and a single counterexample.

Comment by Reid Draper [ 17/May/14 12:44 PM ]

I'm still not sure what this would look like using a nested-property. Can you please write the code the way you'd like to see it as a nested property? This will involve at least two for-alls.

Comment by Maciej Jaśkowski [ 18/May/14 4:56 PM ]

Sure!

I would like to write something like that:

(prop/for-all [matrix (gen-matrix 6)]
  (prop/for-all [random-path (gen-random-path matrix)]
    (<= (cost (tsp matrix)) (cost random-path))))
Comment by Maciej Jaśkowski [ 23/May/14 6:22 AM ]

ping!

Comment by Reid Draper [ 26/May/14 11:47 AM ]

Ok, let's translate that to using gen/bind. Just like with nested properties, bind allows you to create generators that depend on the value of another generator. For example, your gen-random-path depends on the previously generated matrix variable. So let's write a generator that returns both a matrix and a random-path.

(def matrix-and-path
  "Return a two-tuple of [matrix random-path]"
  (gen/bind (gen-matrix 6)
            (fn [matrix]
               (gen/tuple (gen/return matrix) (gen-random-path matrix)))))

(prop/for-all [[matrix random-path] matrix-and-path]
  (<= (cost (tsp matrix)) (cost random-path))




[TCHECK-6] Rose-filter may assume too much Created: 04/Mar/14  Updated: 07/Aug/14

Status: Open
Project: test.check
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Jean Niklas L'orange