<< Back to previous view

[NREPL-69] Interrupt of load-file generates java.lang.ThreadDeath exception Created: 02/Nov/14  Updated: 02/Nov/14

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

Type: Defect Priority: Major
Reporter: Vitalie Spinu Assignee: Chas Emerick
Resolution: Unresolved Votes: 1
Labels: errormsgs, interop, stacktrace
Environment:

Clojure 1.6.0, (both CIDER and lein's REPL-y 0.3.5). No problem with Clojure 1.5.1.



 Description   

At lein prompt (load-file "src/test/core.clj") where core.clj contains (Thread/sleep 10000) then C-c to interrupt.

{{
CompilerException java.lang.ThreadDeath, compiling/home/vitoshka/tmp/cidertest/src/blabla/core.clj:1:17)
clojure.lang.Compiler.load (Compiler.java:7142)
clojure.lang.Compiler.loadFile (Compiler.java:7086)
clojure.lang.RT$3.invoke (RT.java:318)
user/eval3418 (form-init8959987907704530559.clj:1)
clojure.lang.Compiler.eval (Compiler.java:6703)
clojure.lang.Compiler.eval (Compiler.java:6666)
clojure.core/eval (core.clj:2927)
clojure.main/repl/read-eval-print-6625/fn-6628 (main.clj:239)
clojure.main/repl/read-eval-print--6625 (main.clj:239)
clojure.main/repl/fn--6634 (main.clj:257)
clojure.main/repl (main.clj:257)
clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn--594 (interruptible_eval.clj:67)
Caused by:
ThreadDeath
java.lang.Thread.stop (Thread.java:836)
clojure.tools.nrepl.middleware.interruptible-eval/interruptible-eval/fn--636 (interruptible_eval.clj:204)
clojure.tools.nrepl.middleware/wrap-conj-descriptor/fn--408 (middleware.clj:17)
clojure.tools.nrepl.middleware.pr-values/pr-values/fn--573 (pr_values.clj:17)
clojure.tools.nrepl.middleware/wrap-conj-descriptor/fn--408 (middleware.clj:17)
clojure.tools.nrepl.middleware.load-file/wrap-load-file/fn--751 (load_file.clj:77)
clojure.tools.nrepl.middleware/wrap-conj-descriptor/fn--408 (middleware.clj:17)
clojure.tools.nrepl.middleware.session/add-stdin/fn--710 (session.clj:235)
clojure.tools.nrepl.middleware/wrap-conj-descriptor/fn--408 (middleware.clj:17)
cider.nrepl.middleware.test/wrap-test/fn--3003 (test.clj:199)
clojure.tools.nrepl.middleware/wrap-conj-descriptor/fn--408 (middleware.clj:17)
cider.nrepl.middleware.stacktrace/wrap-stacktrace/fn--2909 (stacktrace.clj:146)
}}

In CIDER, eval the buffer with C-c C-k which uses load-file op and then C-c C-b to interrupt. The relevant CIDER issue is here.






[DJSON-16] Add positional tracking to JSON reader Created: 18/May/14  Updated: 06/Jun/14  Resolved: 06/Jun/14

Status: Closed
Project: data.json
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Tim Clemons Assignee: Stuart Sierra
Resolution: Declined Votes: 0
Labels: enhancement, errormsgs, patch

Attachments: Text File 0001-Explicity-define-whitespace-characters.patch     Text File 0002-Add-position-tracking-to-reader.patch     Text File 0003-Add-line-and-column-information-to-read-exceptions.patch     Text File 0004-Add-stack-of-structure-starting-points-to-reader.patch     Text File 0005-Add-track-pos-argument-to-read.patch     Text File 0006-Replace-instances-of-printf-with-format.patch    
Patch: Code and Test

 Description   

The attached patches add an optional argument to clojure.data.json/read, :track-pos?, that causes the line and column number information for each array and object member to be stored as metadata on the result. Line and column numbers are also added to the various exception messages. Useful for doing validation on a JSON file so that the user can mor easily determine where a problem exists.



 Comments   
Comment by Tim Clemons [ 27/May/14 4:37 PM ]

FYI, it appears my Contributor Agreement has been received and processed: http://clojure.org/contributing

Comment by Stuart Sierra [ 06/Jun/14 3:24 PM ]

I'm not opposed to this feature in principle, but I do not want to take the performance hit of this patch: more than 5X slower in my tests, regardless of whether or not you use the feature.





[CTYP-235] Warn on duplicate defalias' Created: 24/Jun/15  Updated: 20/Jul/15

Status: Open
Project: core.typed
Component/s: Core type system
Affects Version/s: None
Fix Version/s: Backlog

Type: Enhancement Priority: Minor
Reporter: Ambrose Bonnaire-Sergeant Assignee: Ambrose Bonnaire-Sergeant
Resolution: Unresolved Votes: 0
Labels: errormsgs, newbie


 Description   

Defining an alias with the different types should yield a warning.

This should say

(defalias F Any)
(defalias F Int)
;; WARNING: Type alias user/F changed. Old: Any, New: Int

However, if the annotations are = after parse-type, then it should not give a warning.

(defalias G Int)
(defalias G Int)
;; <No warning>





[CMEMOIZE-10] Odd deprecation warning in face of correct API usage? Created: 29/Nov/13  Updated: 12/Nov/14  Resolved: 12/Nov/14

Status: Closed
Project: core.memoize
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Magnar Sveen Assignee: Fogus
Resolution: Declined Votes: 0
Labels: documentation, errormsgs
Environment:

[org.clojure/core.memoize "0.5.6"]



 Description   

Calling

(clojure.core.memoize/ttl get-optimized-assets {} :ttl/threshold ms)

Results in this

WARNING - Deprecated construction method for lu cache; prefered way is: (clojure.core.memoize/lu function <base> <:lu/threshold num>)

This breaks my expectation in two ways:

  • If I'm using `ttl` wrong, I would expect a deprecation warning for that. Now I'm a little stuck.
  • If I'm using `ttl` right, I wouldn't expect a deprecation warning.

Now I am just confused. Am I using a deprecated API? In that case, what is the right one to use? Looking at the source code, I see

Please use clojure.core.memoize/ttl instead.

Which is what I am using, as far as I can tell.

Maybe not a very important issue, but it has me confused and unsure.



 Comments   
Comment by Magnar Sveen [ 03/Dec/13 12:12 AM ]

Turns out I got bit by the JVMs global namespace. Once I included 0.5.6, the ring-middleware-format in our stack also started using that updated version - with the wrong API.

Is it only node.js that has this dependency thing figured out properly?

Sorry about the misleading issue. I'd close it, but I don't have the rights to do so.





[CLJS-1310] ns libspec error message misses :import Created: 14/Jun/15  Updated: 14/Jul/15  Resolved: 14/Jul/15

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 0.0-3308
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: errormsgs
Environment:

node repl


Attachments: Text File CLJS-1310.patch    
Patch: Code

 Description   

Let's say you misspell :import in an ns form. You will get an error message that doesn't indicate that :import is a supported libspec:

cljs.user=> (ns foo.bar (:impert [goog Timer]))
clojure.lang.ExceptionInfo: Only :refer-clojure, :require, :require-macros, :use and :use-macros libspecs supported at line 1 <cljs repl> {:file "<cljs repl>", :line 1, :column 1, :tag :cljs/analysis-error}
	at clojure.core$ex_info.invoke(core.clj:4591)
	at cljs.analyzer$error.invoke(analyzer.cljc:405)
	at cljs.analyzer$error.invoke(analyzer.cljc:403)
	at cljs.analyzer$eval1819$fn__1821$fn__1825.invoke(analyzer.cljc:1569)
	at clojure.core.protocols$fn__6519.invoke(protocols.clj:148)
	at clojure.core.protocols$fn__6476$G__6471__6485.invoke(protocols.clj:19)
	at clojure.core.protocols$seq_reduce.invoke(protocols.clj:31)
	at clojure.core.protocols$fn__6502.invoke(protocols.clj:81)
	at clojure.core.protocols$fn__6450$G__6445__6463.invoke(protocols.clj:13)
	at clojure.core$reduce.invoke(core.clj:6515)
	at cljs.analyzer$eval1819$fn__1821.invoke(analyzer.cljc:1566)
	at clojure.lang.MultiFn.invoke(MultiFn.java:251)
	at cljs.analyzer$analyze_seq.invoke(analyzer.cljc:1943)
	at cljs.analyzer$analyze$fn__2069.invoke(analyzer.cljc:2035)
	at cljs.analyzer$analyze.invoke(analyzer.cljc:2028)
	at cljs.repl$evaluate_form.invoke(repl.cljc:429)
	at cljs.repl$eval_cljs.invoke(repl.cljc:548)
	at cljs.repl$repl_STAR_$read_eval_print__4305.invoke(repl.cljc:823)
	at cljs.repl$repl_STAR_$fn__4311$fn__4318.invoke(repl.cljc:860)
	at cljs.repl$repl_STAR_$fn__4311.invoke(repl.cljc:859)
	at cljs.compiler$with_core_cljs.invoke(compiler.cljc:982)
	at cljs.repl$repl_STAR_.invoke(repl.cljc:825)
	at cljs.repl$repl.doInvoke(repl.cljc:941)
	at clojure.lang.RestFn.invoke(RestFn.java:410)
	at user$eval4488.invoke(NO_SOURCE_FILE:3)
	at clojure.lang.Compiler.eval(Compiler.java:6792)
	at clojure.lang.Compiler.eval(Compiler.java:6755)
	at clojure.core$eval.invoke(core.clj:3079)
	at clojure.main$eval_opt.invoke(main.clj:289)
	at clojure.main$initialize.invoke(main.clj:308)
	at clojure.main$null_opt.invoke(main.clj:343)
	at clojure.main$main.doInvoke(main.clj:421)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)


 Comments   
Comment by David Nolen [ 14/Jul/15 5:46 AM ]

fixed https://github.com/clojure/clojurescript/commit/8d53b008cc60c622f6b4280b38dd96fbd1517ace





[CLJS-994] print a warning when :externs file paths can't be found. Created: 30/Jan/15  Updated: 05/Aug/15

Status: Reopened
Project: ClojureScript
Component/s: None
Affects Version/s: 1.7.48
Fix Version/s: Next

Type: Enhancement Priority: Major
Reporter: Crispin Wellington Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: cljs, enhancement, errormsgs, patch,
Environment:

Linux 64bit

java version "1.7.0_65"
OpenJDK Runtime Environment (IcedTea 2.5.3) (7u71-2.5.3-0ubuntu0.14.04.1)
OpenJDK 64-Bit Server VM (build 24.65-b04, mixed mode)


Attachments: Text File clojurescript-extern-missing-warning.patch    
Patch: Code

 Description   

clojurescript silently ignores missing externs files possibly leading a developer to chase their tail.

Presently it can be very confusing using advanced compilation if you have made a mistake in the path name of one of your :externs files. This patch makes the compiler print a warning on stderr so you can quickly determine the cause of the broken advanced compilation output.

As a side effect, when doing a basic lein-cljsbuild a warning is always printed:

```
WARNING: js resource path closure-js/externs does not exist
```

This is because lein-cljsbuild quietly adds this extra path to your :externs listing without you knowing.



 Comments   
Comment by David Nolen [ 31/Jan/15 1:59 PM ]

You need to bind *out* to *err*, or just print to it directly a la cljs.util/debug-prn.

Comment by Crispin Wellington [ 31/Jan/15 7:30 PM ]

I did bind out to err. Check the patch.

Comment by David Nolen [ 01/Feb/15 12:30 PM ]

Crispin, oops sorry you are correct. Thanks.

Comment by David Nolen [ 13/Mar/15 7:33 AM ]

fixed https://github.com/clojure/clojurescript/commit/5f66a78bf469a9875e51aa39c29d3e66ce890eb4

Comment by David Nolen [ 14/Mar/15 5:55 AM ]

The solution does not work for cljsbuild. It's unclear why there so much machinery in place over the approach taken for deps.clj.

Comment by David Nolen [ 15/Mar/15 10:37 AM ]

Stalled on this cljsbuild issue https://github.com/emezeske/lein-cljsbuild/issues/383

Comment by Crispin Wellington [ 23/Mar/15 2:50 AM ]

This lein-cljsbuild issue is what made me make it just a warning initially, and not a hard error like raising IllegalArgumentException does. Though I agree it should be a hard error. If we start with a warning, it enables the immediate problem for the developer to be resolved, and leaves a wart that the cljs-build project can then see that need fixing on their end. Then when that end is fixed it could be made a hard error. If cljsbuild is fixed fairly soon then all is well, but if it takes a long time, a warning might be a good first step.





[CLJS-967] "java.net.ConnectException: Connection refused" when running node repl Created: 07/Jan/15  Updated: 12/Feb/15  Resolved: 12/Feb/15

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

Type: Defect Priority: Major
Reporter: Brian Muhia Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: cljs, errormsgs
Environment:

Processor: 1.33GHz Intel Atom, Ubuntu 12.04, OpenJDK 1.7, Clojure 1.6.0, ClojureScript 0.0-2665, nodejs v0.10.26


Attachments: Text File backtrace.txt    

 Description   

I used trampoline like so:

rlwrap lein trampoline run -m clojure.main scripts/repl.clj

run the script repl.clj with contents:

(require
  '[cljs.repl :as repl]
  '[cljs.repl.node :as node])

(repl/repl* (node/repl-env)
  {:output-dir "out"
   :optimizations :none
   :cache-analysis true                
   :source-map true})

Instead of getting a repl, I got the exception: Exception in thread "main" java.net.ConnectException: Connection refused, compiling/home/brian/projects/essence-cljs-redux/scripts/repl.clj:3:30)

The full stack trace is attached in backtrace.txt.



 Comments   
Comment by David Nolen [ 08/Jan/15 7:21 AM ]

The issue is that we use a 300ms timeout to connect to the Node.js process, we need to instead redirect the process out and wait for the Node.js REPL server start string.

Comment by David Nolen [ 12/Feb/15 7:22 AM ]

fixed https://github.com/clojure/clojurescript/commit/a0d69a31e371af798647ba9e581831ceb6ea09cc





[CLJS-878] Missing preamble file causes unhelpful stack trace Created: 30/Oct/14  Updated: 02/Dec/14  Resolved: 31/Oct/14

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

Type: Defect Priority: Trivial
Reporter: Darrick Wiebe Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: cljs, errormsgs


 Description   

I think it would be helpful to throw an exception with the missing file path rather than the current

java.lang.IllegalArgumentException: No implementation of method: :make-reader of protocol: #'clojure.java.io/IOFactory found for class: nil
.

Right now you need to work your way down about 10 lines into the stacktrace to find

cljs.closure$preamble_from_paths$fn__3097.invoke(closure.clj:526)
which is the only hint as to what has happened.



 Comments   
Comment by David Nolen [ 31/Oct/14 2:42 AM ]

This is a simple enhancement, a patch is welcome. Thanks.

Comment by David Nolen [ 31/Oct/14 2:44 AM ]

Actually this already appears fixed in master https://github.com/clojure/clojurescript/commit/9fd6bf5bd55421c3d5becacc5230ed661d6fb3c3

Comment by David Nolen [ 31/Oct/14 2:45 AM ]

See CLJS-869





[CLJS-875] Compiler error on :" Created: 22/Oct/14  Updated: 05/Nov/14  Resolved: 05/Nov/14

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

Type: Defect Priority: Major
Reporter: Stuart Mitchell Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: compiler, errormsgs
Environment:

Ubuntu 14.04
org.clojure/clojurescript "0.0-2277"



 Description   

(str "this will break" :"the parser")
This simple form (note the misplaced ':' causes the compiler to barf with the following error
Caused by: clojure.lang.ExceptionInfo: EOF while reading string

no mention of the ':' and in larger blocks of code the unmatched brackets several lines away is flagged

in clojure the error is
RuntimeException Invalid token: : clojure.lang.Util.runtimeException (Util.java:221)
RuntimeException Unmatched delimiter: ) clojure.lang.Util.runtimeException (Util.java:221)

Note it mentions the errant ':'



 Comments   
Comment by Stuart Mitchell [ 22/Oct/14 11:04 PM ]

I would like the error reporting from the compiler improved in this case, so that it mentions that the problem is caused by the ':'.

Comment by Nicola Mometto [ 23/Oct/14 7:53 AM ]

This appears to be an issue caused by tools.reader, I'm working on a fix for it

Comment by Nicola Mometto [ 23/Oct/14 9:17 AM ]

I just pushed a 0.8.10 release of tools.reader that fixes this issue

Comment by David Nolen [ 05/Nov/14 6:33 AM ]

fixed https://github.com/clojure/clojurescript/commit/ecb13f06e766182e1daa7d227def3bad98bd8c49





[CLJS-784] PersistHashMap's -conj implementation recurses infinitely if element to be conjed is not a vector. Created: 15/Mar/14  Updated: 08/May/14  Resolved: 08/May/14

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

Type: Defect Priority: Minor
Reporter: Alex Coventry Assignee: Michał Marczyk
Resolution: Completed Votes: 0
Labels: errormsgs
Environment:

Happens on cljsfiddle, among other environments.


Attachments: Text File 0001-CLJS-784-make-conj-on-maps-behave-as-it-does-in-Cloj.patch     Text File 0002-CLJS-784-Fix-Map.-conj-for-map-entry-seqs-that-don-t.patch     Text File 0003-CLJS-784-use-reduce-in-conj-on-maps.patch    

 Description   

In commit b8681e8 the implementation is

ICollection
  (-conj [coll entry]
    (if (vector? entry)
      (-assoc coll (-nth entry 0) (-nth entry 1))
      (reduce -conj coll entry)))

Thus, e.g., (-conj {} "foo") results in an infinite recursion, and a stack overflow. This causes things like (merge {} "foo") to fail for the same reason.

Not sure what the purpose of the not-vector branch could be. I can't think of a situation where it would give a useful result. Maybe it could throw a more helpful error message.



 Comments   
Comment by Michał Marczyk [ 22/Apr/14 6:13 AM ]

This actually applies to all three map types. (In fact, in (-conj {} "foo"), the map is an array map.) In Clojure, conj on a map works with a number of argument types:

1. map entries;

2. two-element vectors;

3. seqables of map entries.

The final case is, perhaps surprisingly, the oldest one. Merging maps falls under it, since for map arguments it boils down to merge minus special treatment of nil (merge uses conj to merge pairs of maps); but arbitrary seqables of map entries are supported. (NB. these must be actual map entries, not two-element vectors!) This allows one, for example, to filter a map and conj the result of that into another map.

So, we want to support the legitimate use cases while maybe complaining about code that wouldn't work in Clojure if it's not too much of a problem performance-wise. An example of a call that we'd probably like to throw: {{(conj {} (list (list [:foo 1])))}}.

The attached patch makes the -conj implementations in all the map types use an explicit loop in the non-vector branch and adds some test for the resulting behaviour.

Comment by David Nolen [ 05/May/14 5:07 PM ]

fixed https://github.com/clojure/clojurescript/commit/3d4405b9b22d36e2e686a084c54ae3f6e5a6208a

Comment by Herwig Hochleitner [ 06/May/14 6:11 AM ]

With patch 0001 (3d4405b), map conj fails for seqs, that don't implement -next.

(merge {:a 1} (hash-map :b 2))
;;=> Error: No protocol method INext.-next defined for type cljs.core/NodeSeq: ([:b 2])
...
at cljs.core.PersistentArrayMap.cljs$core$ICollection$_conj$arity$2 (http://localhost:6030/:15569:42)
...
Comment by Herwig Hochleitner [ 06/May/14 6:46 AM ]

I guess implementing INext is not part of the contract for ISeqable.-seq, which means NodeSeq doesn't have to implement it, right?
In that case, the right fix is to use next instead of -next inside of Map.-conj, when dealing with a (possibly user defined) seq of MapEntries.

Attached patch 0002 uses next instead of -next and adds tests for map-entry seqs not implementing INext

Comment by Michał Marczyk [ 06/May/14 3:04 PM ]

Good catch, thanks!

Another approach would be to use reduce, hopefully benefiting from IReduce speed boosts. Of course we'd need to use a custom reduction function wrapping -conj with a vector? check. The attached patch implements this.

Comment by Michał Marczyk [ 06/May/14 3:49 PM ]

Actually, scratch the part about IReduce speed boosts – sorry for the confusion!

Having run better benchmarks with the two patches on a recent build of V8 and I have to say that there doesn't seem to be much of a difference and actually the next-based approach comes out ahead sometimes. In Clojure, a hand-rolled loop-based "map-seq-conj" loses to a hand-rolled reduce-based impl consistently, as far as I can tell, although only by ~3-5%. I've been conj-ing seqs over vectors of vectors, which should be friendly to reduce.

Comment by Herwig Hochleitner [ 08/May/14 4:10 AM ]

Despite no direkt speed boost in benchmarks, I'm fond of using reduce here. GC Pressure is hard to benchmark.

Comment by David Nolen [ 08/May/14 6:18 PM ]

going to go with the next based patch.

Comment by David Nolen [ 08/May/14 6:18 PM ]

fixed https://github.com/clojure/clojurescript/commit/c1a29f1eceae9d1f1f637d9c5f2fa132efa58c47





[CLJS-783] Confusing error messages when ns compilation fails due to a missing dependency Created: 11/Mar/14  Updated: 17/Apr/14  Resolved: 17/Apr/14

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

Type: Defect Priority: Minor
Reporter: Michael Klishin Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: error-reporting, errormsgs, usability


 Description   

I have a namespace proj.a which requires proj.b. proj.b, in turn, relies on core.async. I did not have
core.async listed in project.clj by accident, and the resulting error message was

goog.require could not find: proj.b.

This is not incredibly helpful. I've wasted over an hour trying to understand why one ns in my project
cannot reference another one.

Expected outcome: compilation must fail instead of swallowing exceptions. If it matters, I use lein-cljsbuild 1.0.2.



 Comments   
Comment by David Nolen [ 12/Mar/14 8:36 AM ]

And which version of ClojureScript are you using?

Comment by Michael Klishin [ 12/Mar/14 8:52 AM ]

0.0-2138

Comment by Michael Klishin [ 12/Mar/14 8:53 AM ]

I strongly disagree with the severity change. Anything that can waste beginners hours of time is not a minor priority.

Comment by David Nolen [ 12/Mar/14 10:18 AM ]

That is a fairly old release of ClojureScript, can you replicate the issue with 0.0-2173? When you change your dependency please make sure to run "lein cljsbuild clean" first.

Comment by David Nolen [ 17/Apr/14 3:53 PM ]

Closing unless I hear step on how to reproduce this in more recent ClojureScript releases. Feel free to request a re-open if you can demonstrate that this isn't resolved.





[CLJS-685] Cannot call method 'fromArray' of undefined -- Clojurescript 0.0-2030 Created: 17/Nov/13  Updated: 26/Nov/13  Resolved: 22/Nov/13

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

Type: Defect Priority: Minor
Reporter: John Chijioke Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: Compiler, bug, errormsgs
Environment:

Linux 3.2.0-52-generic x86_64 GNU/Linux, java 1.7, clojure 1.5.1



 Description   

Clojurescript 0.0-2030

This line from compile cljs.core is causing problems:

cljs.core.PersistentQueue.EMPTY = (new cljs.core.PersistentQueue(null, 0, null, cljs.core.with_meta(cljs.core.PersistentVector.EMPTY, cljs.core.PersistentArrayMap.fromArray([new cljs.core.Keyword(null, "end-line", "end-line", 2693041432), 3820, new cljs.core.Keyword(null, "end-column", "end-column", 3799845882), 69], true)), 0));

error message: Uncaught TypeError: Cannot call method 'fromArray' of undefined.

That's the first mention of fromArray in that file. I don't know if it's an ordering problem.



 Comments   
Comment by John Chijioke [ 17/Nov/13 11:10 PM ]

I solved it by replacing [] with cljs.core.PersistentVector.EMPTY. I think this must be a reader problem.

Comment by David Nolen [ 17/Nov/13 11:32 PM ]

This ticket needs more details, how can this error be reproduced?

Comment by Peter Taoussanis [ 22/Nov/13 3:03 AM ]

Hi, I'm seeing the same problem with tools.reader 0.8.0.

Any Clojurescript file (even an empty file) will produce the error.

Clojure: 1.6.0-alpha2
Clojurescript: 0.0-2030
Cljsbuild: 1.0.0
tools.reader: 0.8.0

Tried `lein cljsbuild clean`.

Problem is resolved by dropping back to tools.reader 0.7.10.

Update: have created an issue on the tools.reader GitHub page: https://github.com/clojure/tools.reader/issues/7

Update 2: this isn't something specific to Cljs 0.0-2030 btw, tools.reader 0.8.0 seems to produce the same error against at least Cljs 0.0-2060, 0.0-2027, 0.0-2024.

Comment by Nicola Mometto [ 22/Nov/13 6:49 AM ]

tools.reader 0.8.0 introduces end-column/end-line metadata, this needs to be elided as per line/column to avoid this bootstrapping issue.

Comment by David Nolen [ 22/Nov/13 8:02 AM ]

fixed, http://github.com/clojure/clojurescript/commit/36d401797f85c99794eef8a71239641930c36871

Comment by Peter Taoussanis [ 22/Nov/13 10:30 AM ]

Thanks a lot David, Nicola - much appreciated! Cheers

Comment by John Chijioke [ 26/Nov/13 6:32 AM ]

Thanks David. Cheers!





[CLJS-639] Produce errors when records are initialized incorrectly Created: 27/Oct/13  Updated: 19/Nov/13  Resolved: 19/Nov/13

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

Type: Enhancement Priority: Minor
Reporter: Scott Feeney Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord, errormsgs


 Description   

There are a couple of wrong ways you can use a record initializer that don't produce errors in ClojureScript, just nils:

ClojureScript:cljs.user> (defrecord Thing [foo bar])
cljs.user/Thing
ClojureScript:cljs.user> (Thing. 1 2)  ; correct usage
#cljs.user.Thing{:foo 1, :bar 2}
ClojureScript:cljs.user> (Thing. 1)    ; wrong number of fields
#cljs.user.Thing{:foo 1, :bar nil}
ClojureScript:cljs.user> (Thing 1 2)   ; forgetting the dot
nil

Compare Clojure:

user=> (defrecord Thing [foo bar])
user.Thing
user=> (Thing. 1 2)
#user.Thing{:foo 1, :bar 2}
user=> (Thing. 1)

CompilerException java.lang.IllegalArgumentException: No matching ctor found for class user.Thing, compiling:(/tmp/form-init7089728177345913731.clj:1:1) 
user=> (Thing 1 2)

RuntimeException Expecting var, but Thing is mapped to class user.Thing  clojure.lang.Util.runtimeException (Util.java:219)

It would make debugging easier if ClojureScript followed Clojure's example here and gave a useful error immediately in case of the last 2 examples.



 Comments   
Comment by David Nolen [ 29/Oct/13 6:40 PM ]

Thanks for the report, these warnings would indeed be nice.

Comment by David Nolen [ 19/Nov/13 4:41 PM ]

fixed, http://github.com/clojure/clojurescript/commit/8c6dd2468a3913e316d021fc0b09745bd3ac7dcd

Comment by David Nolen [ 19/Nov/13 4:43 PM ]

Wrong arity constructor issue fixed. Separate ticket need for invoking ClojureScript ctors as functions.





[CLJ-1797] Mention cljc when require fails Created: 10/Aug/15  Updated: 10/Aug/15

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

Approval: Triaged

 Description   

If you attempt to require a namespace for which the code cannot be located, now that cljc files are considered when locating code, it could be mentioned in the list of files considered on the classpath.

$ java -jar clojure-1.7.0.jar 
Clojure 1.7.0
user=> (require 'foo.bar)
FileNotFoundException Could not locate foo/bar__init.class or foo/bar.clj on classpath.  clojure.lang.RT.load (RT.java:449)

Note: FWIW, ClojureScript has similar error messages, and the order listed in the error message is the order tried when loading. If this were followed, I suspect the text of the error message above would end up looking like:

Could not locate foo/bar__init.class, foo/bar.clj, or foo/bar.cljc on classpath.





[CLJ-1737] Omit java exception class from CompilerException message Created: 23/May/15  Updated: 23/May/15

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

Type: Enhancement Priority: Minor
Reporter: John Hume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, errormsgs, patch, usability

Attachments: Text File clearer-CompilerException-messase.patch     File compiler_exception_examples.clj    
Patch: Code

 Description   

A CompilerException is always created with a cause exception. Currently the message is built using cause.toString(), which for all examples I've examined is the cause class, followed by a colon, followed by the cause message. In all those examples, the message of the cause is informative, and the class name provides no additional help.

I propose to switch to using cause.getMessage() rather than cause.toString(). This would make it easier for tools to present compiler errors that don't leak implementation details that may confuse a new user. The cause class would still be shown in the stack trace.

Here are the examples I looked at, with the output from before the attached patch:

Example source '(ns foo)

(def'
Exception message:
 java.lang.RuntimeException: EOF while reading, starting at line 3, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:3:1)

Example source ':foo}'
Exception message:
 java.lang.RuntimeException: Unmatched delimiter: }, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:6)

Example source 'foo'
Exception message:
 java.lang.RuntimeException: Unable to resolve symbol: foo in this context, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:14:1)

Example source 'clojure.core/firstt'
Exception message:
 java.lang.RuntimeException: No such var: clojure.core/firstt, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:15:1)

Example source '(nil 1)'
Exception message:
 java.lang.IllegalArgumentException: Can't call nil, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '("hi" 1)'
Exception message:
 java.lang.ClassCastException: java.lang.String cannot be cast to clojure.lang.IFn, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '{:foo}'
Exception message:
 java.lang.RuntimeException: Map literal must contain an even number of forms, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:7)

Example source '1st'
Exception message:
 java.lang.NumberFormatException: Invalid number: 1st, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

And the output with the attached patch applied:

Example source '(ns foo)

(def'
Exception message:
 EOF while reading, starting at line 3, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:3:1)

Example source ':foo}'
Exception message:
 Unmatched delimiter: }, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:6)

Example source 'foo'
Exception message:
 Unable to resolve symbol: foo in this context, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:14:1)

Example source 'clojure.core/firstt'
Exception message:
 No such var: clojure.core/firstt, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:15:1)

Example source '(nil 1)'
Exception message:
 Can't call nil, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '("hi" 1)'
Exception message:
 java.lang.String cannot be cast to clojure.lang.IFn, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '{:foo}'
Exception message:
 Map literal must contain an even number of forms, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:7)

Example source '1st'
Exception message:
 Invalid number: 1st, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)





[CLJ-1734] Display more descriptive error message when trying to use reader conditionals in a non-cljc file Created: 19/May/15  Updated: 20/May/15

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

Type: Enhancement Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, reader


 Description   

I spent a few puzzled minutes trying to understand the following message from the Clojure compiler:

CompilerException java.lang.RuntimeException: Conditional read not allowed, compiling: <filename>

Eventually I realised it was because I was trying to use reader conditionals in a .clj file that I hadn't renamed to cljc. I think it would be really helpful for people working in mixed clj and cljc codebases to have this error message extended to something like:

"Conditional read not allowed because file does not have extension .cljc"



 Comments   
Comment by Alex Miller [ 19/May/15 11:45 PM ]

The reader doesn't know this - it can be called in multiple ways (from repl, via clojure.core/read, via clojure.core/read-string, load/compile .cljc, load/compile .clj) so that description would actually be wrong in some of those. It seems like you're getting a pretty good error message already - it told you the problem and gave you the file name.

The message could be tweaked to something like "Reader conditionals not allowed in this context" which might give you a better clue.

Comment by Daniel Compton [ 20/May/15 3:12 PM ]

Perhaps I'm not understanding how the reader determines whether reader conditionals are allowed or not, but those would all seem to have different reasons for not being allowed and would be caught by different checks. Each of these checks could give a more specific warning explaining why the read wasn't allowed?

Counteracting my point, it looks like there is only one place where this exception is thrown - https://github.com/clojure/clojure/blob/7b9c61d83304ff9d5f9feddecf23e620c0b33c6e/src/jvm/clojure/lang/LispReader.java#L1406. I'm not sure if this could be extended to give more details in different error cases or if that information isn't available at that point?

Comment by Alex Miller [ 20/May/15 3:36 PM ]

The reader is invoked with an options map which will (or will not) have {:read-cond :allow} or {:read-cond :preserve}. That's the only info the reader has - if either of those is set and a reader conditional is encountered, it throws.

The compiler decides how to initialize these options when it's calling the reader. Users of read and read-string similarly decide which options are allowed when they call it. It would be possible to pass more info into the reader or to catch and rethrow in the compiler where more context is known, but both of those complicate the code for what is already a decent error imho.

Comment by Daniel Compton [ 20/May/15 4:03 PM ]

I agree it is a reasonable error message, I guess we can wait and see how other people find it once 1.7 is released. If it turns out to be an issue for lots of other people then we could revisit this then?





[CLJ-1705] vector-of throws NullPointerException if given unrecognized type Created: 14/Apr/15  Updated: 06/May/15

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

Type: Defect Priority: Minor
Reporter: John Croisant Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: checkargs, errormsgs, newbie
Environment:

MacOS X Version 10.9.5

java version "1.8.0_11"
Java(TM) SE Runtime Environment (build 1.8.0_11-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.11-b03, mixed mode)


Attachments: Text File clj-1705-2.patch     Text File clj-1705-3.patch     Text File CLJ-1705.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Summary:

If the user passes an unrecognized type keyword to vector-of, it will throw a NullPointerException with no message. This gives no indication to the user of what the problem is, which can frustrate the user and make debugging harder than it needs to be.

Example:

user=> (vector-of :integer 1 2 3) ; user meant (vector-of :int 1 2 3)

NullPointerException   clojure.core/vector-of (gvec.clj:472)

Approach: Throw more informative error message than NPE with more info.

After:

user=> (vector-of :integer 1 2 3)
IllegalArgumentException Unrecognized type :integer  clojure.core/vector-of (gvec.clj:498)

Patch: clj-1705-3.patch



 Comments   
Comment by Johan Mena [ 05/May/15 12:12 AM ]

Here’s my proposed solution: https://github.com/jhn/clojure/commit/0522fffd7893e8c79969f5c6ac91a333484c8e23 — It works as expected and the tests pass.

One more thing I'm not sure about is how to create a patch for this. The ticket mentions "Release 1.4, Release 1.6” as affected versions, but I found this to still be present in 1.7 and in 1.5 too. Do I checkout only these two tags (1.4, 1.6) from the git repo and create a patch for each one separately? Or what's the usual way to go about it? I read through the Issue Tracking section of the wiki but it's still not very clear.

Thanks.

Johan

Comment by Alex Miller [ 05/May/15 7:09 AM ]

in general, just work from master. We don't generally back port to older versions.

You should make a patch following the instructions at http://dev.clojure.org/display/community/Developing+Patches

Comment by Andy Fingerhut [ 05/May/15 10:20 AM ]

Johan, are you listed as "johan (jhn)" on the contributor list here? http://clojure.org/contributing

If so, it would be good to update that listing to something like "Johan Mena (jhn)" for the time when someone needs to check that you have signed a Clojure CA, before your patch is committed.

Comment by Alex Miller [ 05/May/15 10:35 AM ]

I believe that's correct - I have updated the contributing page.

Comment by Johan Mena [ 05/May/15 1:57 PM ]

The patch I attached yesterday did start from master, so I think it's good to go for review.

Thanks Alex & Andy!

Comment by Alex Miller [ 06/May/15 9:56 AM ]

Ok, I looked at the patch. Functionally, seems ok other than that the find+destructuing doesn't seem to have a use over get.

However, I also looked at performance for the happy path using criterium and the following test:

(quick-bench (vector-of :int 1 2))

Before the patch: 27.418350 ns
After the patch: 79.883695 ns

The reason for this is that the prior code created a single map and constructed the array managers in there once, whereas the patch causes the array manager to be created each time through the code.

Re-extracting the map and replacing the find with get, I was back to: 34.320916 ns

I think that's too much of a hit for this change so I looked at a couple variants:

  • def'ing each am separately and using a `case` expression - 36.566750 ns
  • using `or` instead of if-let - 29.680252 ns

I think that last one is pretty close, but we're paying a hit just to invoke the new function, so my final stab was turning that into a macro - 28.411626 ns, which seems tolerable to me. Because I used a macro, I also had to move the type hints in vector-of to avoid reflection.

Since I had everything sitting here, I went ahead and made a new patch. I feel bad about replacing yours, but not sure what else makes sense to do.

Comment by Alex Miller [ 06/May/15 10:02 AM ]

Ok, I split up the patch into test and code commits so you have credit for some of the changes! Since it's got my stuff in here, this will have to wait till it gets added to 1.8 to get screened by someone else.

Comment by Johan Mena [ 06/May/15 11:04 AM ]

Haha, you shouldn't have bothered, but thanks!

And thanks for the detailed explanation! Actually 'get' was my first approach too but the (get map key not-found) form kept throwing the exception in the `not-found` part even in the happy case. I asked around and found out the else part needs to be evaluated in this case too. Someone suggested using a macro but I'm not very familiar with that just yet, so I went with if-let, which seemed readable. Just out of curiosity, when you tried the get approach, did you use (get map key) and check for nil to throw the exception?

I ran my code through the irc channel and got positive feedback, so completely forgot about performance. Will keep it in mind next time.





[CLJ-1672] Better error message when passing a list to update-in Created: 11/Mar/15  Updated: 11/Mar/15

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

Type: Enhancement Priority: Minor
Reporter: John Gabriele Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: checkargs, errormsgs
Environment:

OpenJDK 1.7 on GNU/Linux



 Description   

This one confused me when I'd accidentally passed a list (returned by a function) in to `update-in` instead of a vector.

Example:

some-app.core=> (update-in [:a :b :c] [1] name)
[:a "b" :c]
some-app.core=> (update-in '(:a :b :c) [1] name)

NullPointerException   clojure.core/name (core.clj:1518)

Similar result if passing in another function; for example:

some-app.core=> (update-in ["a" "b" "c"] [1] str/capitalize)
["a" "B" "c"]
some-app.core=> (update-in '("a" "b" "c") [1] str/capitalize)

NullPointerException   clojure.string/capitalize (string.clj:199)


 Comments   
Comment by Alex Miller [ 11/Mar/15 9:26 AM ]

I think this is effectively a dupe of CLJ-1107 re throwing on get with a non-Associative collection?





[CLJ-1668] ns macro throws NPE if empty reference is specified Created: 02/Mar/15  Updated: 02/Mar/15

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

Type: Defect Priority: Minor
Reporter: Philipp Meier Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, macro, namespace


 Description   

The following invocations of `ns` will all throw a NPE

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

;; throw


1. Unhandled java.lang.NullPointerException
   (No message)

                      core.clj: 1518  clojure.core/name
                      core.clj: 5330  clojure.core/ns/process-reference
                      core.clj: 2559  clojure.core/map/fn
                  LazySeq.java:   40  clojure.lang.LazySeq/sval
                  LazySeq.java:   49  clojure.lang.LazySeq/seq
                       RT.java:  484  clojure.lang.RT/seq
                      core.clj:  133  clojure.core/seq
                      core.clj:  694  clojure.core/concat/cat/fn
                  LazySeq.java:   40  clojure.lang.LazySeq/sval
                  LazySeq.java:   49  clojure.lang.LazySeq/seq
                     Cons.java:   39  clojure.lang.Cons/next
                       RT.java: 1654  clojure.lang.RT/boundedLength
                      AFn.java:  148  clojure.lang.AFn/applyToHelper
                      Var.java:  700  clojure.lang.Var/applyTo

I'd expect an exception that is describing the cause of the error, not an "symptom".






[CLJ-1629] Improve error message when defn form omits parameter declaration Created: 29/Dec/14  Updated: 29/Dec/14

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

Type: Enhancement Priority: Major
Reporter: Sanel Zukan Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs
Environment:

Reproducible on all platforms and all clojure versions.


Approval: Triaged

 Description   

When defn form is malformed, Clojure compiler will report meaningless error and in combination with function body, can cause really bad experience. Here is the sample:

(defn foo
  "This is docstring."
  (let [i 1]
    (+ i 1)))

It will report:

IllegalArgumentException Parameter declaration "let" should be a vector  clojure.core/assert-valid-fdecl (core.clj:7123)

However, if is written:

(defn foo "bla")

error report makes more sense:

IllegalArgumentException Parameter declaration missing  clojure.core/assert-valid-fdecl (core.clj:7107)


 Comments   
Comment by Michael Blume [ 29/Dec/14 1:39 PM ]

I don't think this is really meaningless – if you replace the symbol let with a vector, say, [i], you get a perfectly valid function definition

(defn foo
  "This is docstring."
  ([i] [i 1]
    (+ i 1)))
Comment by Sanel Zukan [ 29/Dec/14 2:41 PM ]

Yes and maybe make sense for this case. But in general, the report is misleading for common defn forms (how often you will see function definitions written this way, unless you want multi-arity function) and should have the same report as for second sample; in both cases it is the same cause.





[CLJ-1585] Report boxed math warning on function that boxes primitive return value Created: 11/Nov/14  Updated: 28/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, math

Attachments: Text File clj-1585.patch    
Patch: Code
Approval: Triaged

 Description   

With the new :warn-on-boxed (CLJ-1325), these examples do not report a boxed math warning although they each do boxing:

user=> (defn f1 [^long x] (inc x))
f1
user=> (defn f2 [x] (aget (long-array [1 2]) 0))
f2
user=> (defn f3 [x] (aget (int-array [1 2]) 0))
f3
user=> (defn f4 [^String s] (.indexOf s "a"))

Cause: emitBoxReturn has a hard-coded call to box a prim return value.

Solution: If *unchecked-math* is set to :warn-on-boxed, emit a warning on boxing of primitive numeric return types.

Patch:



 Comments   
Comment by Alex Miller [ 12/Nov/14 12:39 AM ]

Attached patch does the job, but from trying it out on some real code, it finds both problematic cases and lots of cases that could safely be ignored and/or where there is no obvious way to fix the warning. I think it may need some more tuning to reduce the rate of unfixable things a bit.





[CLJ-1568] Incorrect error locations reported in the stacktrace Created: 19/Oct/14  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

Type: Defect Priority: Major
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 19
Labels: errormsgs, ft

Attachments: Text File 0001-CLJ-1568-fix-incorrect-error-locations.patch     Text File 0001-CLJ-1568-fix-incorrect-error-locations-v2.patch     Text File clj-1568.patch    
Patch: Code and Test
Approval: Ok

 Description   

The following code produces an incorrect stacktrace:

(ns clojure-demo.core)

(defn foo
  "I don't do a whole lot."
  [x]
  (println x "Hello, World!"))

(/ 1 0)
Exception in thread "main" java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:6:31)

The problem is actually on the 8th line. As a matter of fact - there's nothing at location 6:31.
This is a pretty serious problem as many tools parse stacktraces for error locations.
Here's a related discussion in cider's issue tracker.

Patch: clj-1568.patch
Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 19/Oct/14 1:39 PM ]

Maybe a dupe of CLJ-1561 ?

Comment by Andy Fingerhut [ 19/Oct/14 4:16 PM ]

I tried out the example given in the description, with the latest Clojure master as of today plus the patch for CLJ-1561 called 0002-Mark-line-number-after-emitting-children.patch, dated Oct 10 2014.

The line:column number 6:31 is the same for that patched version as it is in the ticket description, which is for Clojure 1.6.0.

The issue of misleading line:column numbers is common between the two tickets, but at least the proposed improvement in CLJ-1561's patch is not effective for improving this issue.

Comment by Bozhidar Batsov [ 20/Oct/14 1:36 AM ]

I know that the issue list for 1.7 is pretty much finalised, but I think that this issue and and CLJ-1561 should be fixed as soon as possible.
Correct error reporting is extremely important IMO.

Comment by Nicola Mometto [ 20/Oct/14 8:28 AM ]

Attached a patch that fixes the issue by consuming all the whitespaces before retrieving line/column info for the next form.

Comment by Alex Miller [ 20/Oct/14 8:39 AM ]

Are there possible downsides to more eagerly consuming whitespace as done in the patch?

Comment by Nicola Mometto [ 20/Oct/14 8:44 AM ]

I can't think of any

Comment by Paul Stadig [ 22/Oct/14 2:59 PM ]

The defect on master does not have effect when using compile:

user=> (require 'clojure-demo.core)

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:6:31) 
user=> (load "/clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:6:31) 
user=> (compile "clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(core.clj:8:1) 

With the patch applied all the line numbers are the same in all cases:

user=> (require 'clojure-demo.core)

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:8:1) 
user=> (load "/clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:8:1) 
user=> (compile "clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(core.clj:8:1) 

Agreed that this seems to be orthogonal to CLJ-1561.

Comment by Bozhidar Batsov [ 10/Jan/15 3:08 AM ]

Seems we need to add tests before this can be merged - https://groups.google.com/forum/#!topic/clojure-dev/7pFhG8LMvGo

Comment by Daniel Solano Gómez [ 11/Jan/15 12:20 AM ]

Well, I've been looking into adding tests to this patch, and I've made some interesting discoveries. The additions to Compiler.load() seem to work just fine. However, I'm not seeing much coming out of the changes to Compiler.compile. You can see in Paul's comment above that the compile call actually reports the correct location. I created a test that throws a NullPointerException during compilation, and in the case of compile, it is never wrapped in a CompilerException. I'll attach my test patch that contains the example code I have been working with.

Comment by Daniel Solano Gómez [ 11/Jan/15 12:26 AM ]

Patch with tests for code paths in the fix. The tests for the Compiler.compile changes are not showing what I had expected.

Comment by Alex Miller [ 12/Jan/15 8:07 AM ]

Whenever people feel this is ready for screening, just switch the Approval from Incomplete to Vetted.

Comment by Nicola Mometto [ 12/Jan/15 8:39 AM ]

Updated patch removing the changes to Compiler.compile as they seem to be useless, by Daniel's tests

Comment by Daniel Solano Gómez [ 14/Jan/15 10:26 AM ]

I have cleaned up my test code a bit and put together a combined patch that includes both the fix and the tests.

Comment by Bozhidar Batsov [ 14/Jan/15 11:12 AM ]

Great! Looking forward to seeing this merged.

Comment by Bozhidar Batsov [ 20/Feb/15 7:50 AM ]

Seems we can finally merge this!





[CLJ-1561] Incorrect line numbers are emitted Created: 10/Oct/14  Updated: 20/Mar/15  Resolved: 20/Mar/15

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

Type: Defect Priority: Major
Reporter: Paul Stadig Assignee: Unassigned
Resolution: Completed Votes: 24
Labels: errormsgs

Attachments: Text File clj-1561.patch    
Patch: Code and Test
Approval: Ok

 Description   

The Clojure JVM compiler marks the line number for a form before emitting the children for that form. Marking the line number before emitting children leads to incorrect line numbers when a runtime error occurs. For example, when

 (foo bar
      baz)

is emitted the compiler will visit the line number for the expression, then emit the children expressions ('bar' and 'baz') which will mark their own line numbers, then come back and emit the invoke bytecode for 'foo', but since the last line number to be marked was that of 'baz', if 'foo' throws an exception the line number of 'baz' will be reported instead of the line number for the expression as a whole.

This same issue was being manifested with special forms and inlined functions, and was especially bad in the case of the threading macro '->', because it is usually spread across several lines, and the line number reported could end up being very different than the line actually causing an exception.

A demonstration of the incorrect line numbers (and how the fix affects line numbers) can be seen here https://github.com/pjstadig/clojure-line-numbers

Patch: clj-1561.patch

Screened by: Alex Miller



 Comments   
Comment by Jozef Wagner [ 10/Oct/14 1:57 PM ]

additions in your patch mixes tabs and spaces. Could you please update the patch so that your added lines indent only with tab characters? Not everyone has tab set at 4 spaces...

Comment by Paul Stadig [ 10/Oct/14 2:42 PM ]

There's already a mixture of just tabs, just spaces, and tabs & spaces in Compiler.java. I'm not sure what the "standard" is, but I've changed the patch to match the surrounding lines.

Comment by Paul Stadig [ 10/Oct/14 2:42 PM ]

Patch with whitespace changes.

Comment by Alex Miller [ 20/Oct/14 8:38 AM ]

These changes will affect the line number tables for a variety of Clojure constructs when compiled. It would be very helpful to me to have a set of examples that covered each case touched in the patch so that I could compile them and look at the bytecode vs the source. This would greatly accelerate the screening process.

Comment by Paul Stadig [ 20/Oct/14 2:29 PM ]

Alex,
I have created a repo on github that has a sample file demonstrating the line number changes.

https://github.com/pjstadig/clojure-line-numbers

Hope that helps!

BTW, I'd be glad to do a skype call or hangout, if you have questions.

Comment by Alex Miller [ 20/Oct/14 2:34 PM ]

This is very helpful, thanks!!

Comment by Alex Miller [ 22/Oct/14 11:35 AM ]

In the hunk at 3191 in KeywordInvokeExpr, a call to visitLineNumber was added, but the prior call 4 lines earlier was not removed. Should it be?

Comment by Paul Stadig [ 22/Oct/14 12:05 PM ]

I left that in thinking that if something goes wrong with the getstatic instruction (null pointer exception? class cast exception?) it should report the line number of the KeywordInvokeExpr. It may be that there isn't a realistic possibility that anything could actually happen with that getstatic instruction, but that was the thought process.

My general rule of thumb was if an emit method emits any instructions before it calls the emit method on another expr, then it should mark its line number before and after the recursive emit call (assuming that the recursive emit call would mark its own line number). In cases where an emit method immediately calls another emit method, then I don't bother to mark a line number until afterwards.

Comment by Bozhidar Batsov [ 10/Jan/15 3:09 AM ]

Seems we need to add tests before this can be merged - https://groups.google.com/forum/#!topic/clojure-dev/7pFhG8LMvGo

Comment by Daniel Solano Gómez [ 11/Jan/15 8:42 PM ]

Here's an updated version of Paul's patch that applies cleanly to master.

I based my test code from his GitHub repository, but I have made a few changes:

  1. I have added test cases for when emitUnboxed or the reflecting branch of emit is called.
  2. The original keyword invocation test doesn't actually go through KeywordInvokeExpr. It is resolved by the change to InvokeExpr.emitArgsAndCall, so I have removed it.

Additionally, I have been unable to create test cases that verify that the changes for intrinsic predicates, KeywordInvokExprs, and for the changes inside of InvokeExpr.emit. These could be that they are redundant, but I am not sure.

Comment by Paul Stadig [ 12/Jan/15 5:51 AM ]

Thanks Daniel!

I applied the patches for my commit (as updated by Daniel) and Daniel's commit. The namespace docstring had shifted all the line numbers down by 4, and the tests were failing. I amended Daniel's commit with updated line numbers and combined both commits into a single patch file (clj-1561.patch).

I also reverted my commit and re-ran Daniel's tests to verify that they failed as expected.

I believe the combined patch file should include everything we need other than the difficult test cases that Daniel mentions in his comment.

Cheers!

Comment by Alex Miller [ 12/Jan/15 8:07 AM ]

Whenever people feel this is ready for screening, just switch the Approval from Incomplete to Vetted.

Comment by Paul Stadig [ 12/Jan/15 2:20 PM ]

I have added a test for KeywordInvokeExpr, and a new test to test the logic in InvokeExpr.emit.

I removed the changes I made to StaticMethodExpr.emitIntrinsicPredicate, since I was unable to find a way to create a test case. The issue is that we mark the lines of child expressions, then come back to the parent expression and try to invoke without marking its line, but in the case of intrinsic predicates there is no invocation. When we come back to the parent we directly emit some bytecodes. I don think the change I made in that case actually changed anything, but I removed it since there is no need for it.

Comment by Alex Miller [ 14/Jan/15 10:07 PM ]

Could we squash the patch into a single commit?

Comment by Alex Miller [ 15/Jan/15 9:21 AM ]

Also, in addition to squashing the patch, can we rename examples_clj_1561.clj to line_number_examples.clj or something w/o the ticket in it? I'm ready to mark as screened when those are done. (And if that can get done today, it's likely to get pushed forward tomorrow.)

Comment by Daniel Solano Gómez [ 15/Jan/15 9:24 AM ]

In case Paul doesn't get to it sooner, I can take care of it this afternoon.

Comment by Paul Stadig [ 15/Jan/15 10:40 AM ]

Alex,
I'd like to maintain attribution. I could squash my test related commits into Daniel's commit, so we'd have two commits, my commit for the change and Daniel's for the tests. Would that suffice? If so, I can take care of it now.

Comment by Alex Miller [ 15/Jan/15 11:13 AM ]

Sure, that's fine.

Comment by Alex Miller [ 15/Jan/15 11:14 AM ]

note that one of the commits has a whitespace error (I think Daniel's) that got removed in a later commit. In the end patch, please make sure it doesn't warn about any whitespace errors when applied.

Comment by Alex Miller [ 15/Jan/15 12:46 PM ]

Paul, this patch still seems to have examples_clj_1561.clj in it - would prefer name that is meaningful vs jira issue.

Comment by Paul Stadig [ 15/Jan/15 1:44 PM ]

Yeah, sorry, I'm still working on it. Should be up shortly.

Comment by Paul Stadig [ 15/Jan/15 2:05 PM ]

I squashed the commits down to two: my commit for the change, and Daniel's for the tests. I renamed the "examples_clj_1561" file to "line_number_examples". I removed some trailing whitespace from Daniel's commit. There are still whitespace errors (on my machine) from my commit because tabs are used for indenting. I used tabs to indent because that is how the lines surrounding were formatted, and Jozef had already complained about the formatting. It also appears that these whitespace errors are dependent on your git config. I have:

[core]
whitespace = trailing-space,space-before-tab,tab-in-indent

But when I comment that out of my gitconfig I get no whitespace errors. I guess the bottom line is if we want to agree on a standard config for that I'd be glad to accomodate, but I don't think we want to reformat Compiler.java, so the best choice seems to be to just match the surrounding context, which is what I've done. If you want me to do anything further with that, then let me know.

Ball is back in your court, Alex.





[CLJ-1531] inc always warns when *unchecked-math* is set Created: 23/Sep/14  Updated: 23/Sep/14  Resolved: 23/Sep/14

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

Type: Defect Priority: Major
Reporter: Pierre-Yves Ritschard Assignee: Pierre-Yves Ritschard
Resolution: Not Reproducible Votes: 0
Labels: bug, errormsgs


 Description   

While testing 1.7-alpha2 I stumbled this (affects clojure.data.codec amongst others). inc inlines a call to clojure.lang.Numbers's inc method which according to the rules of CLJ-1325 will warn.

I can't find a way around it for now, except maybe having a primitive-inc and primitive-dec java method which would be inlined in that case.

Happy to work on a patch but would prefer discussing it first.



 Comments   
Comment by Nicola Mometto [ 23/Sep/14 12:42 PM ]

I cannot reproduce this:

Clojure 1.7.0-master-SNAPSHOT
user=> (set! *unchecked-math* true)
true
user=> (inc 1)
2

Looking at Numbers.java I see both unchecked_inc and inc have long/double taking methods.

Comment by Pierre-Yves Ritschard [ 23/Sep/14 1:49 PM ]

you're right, i must have been confused.

Comment by Pierre-Yves Ritschard [ 23/Sep/14 1:50 PM ]

not a bug





[CLJ-1490] Exception on protocol implementation after protocol reloaded could be improved Created: 04/Aug/14  Updated: 07/Nov/14

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: errormsgs, protocols

Attachments: Text File CLJ-1490.1.patch    
Patch: Code and Test
Approval: Triaged

 Description   

In a situation where you define a protocol, and then define a class that extends that protocol (e.g., reify, defrecord, deftype) and then later, re-define the protocol (typically, by reloading the namespace that defines the protocol), then the existing instances are no longer valid.

However, the exception that gets generated can be confusing:

     java.lang.IllegalArgumentException: No implementation of method: :injections of protocol: #'fan.microservice/MicroService found for class: fan.auth.AuthService
                                           clojure.core/-cache-protocol-fn                  core_deftype.clj:  544
                                           fan.microservice/eval23300/fn/G                  microservice.clj:   12
                                                       clojure.core/map/fn                          core.clj: 2559
                                                 clojure.lang.LazySeq.sval                      LazySeq.java:   40
                                                  clojure.lang.LazySeq.seq                      LazySeq.java:   49
                                                    clojure.lang.Cons.next                         Cons.java:   39
                                             clojure.lang.RT.boundedLength                           RT.java: 1654
                                               clojure.lang.RestFn.applyTo                       RestFn.java:  130
                                                        clojure.core/apply                          core.clj:  626
                 fan.microservice.StandardContainer/construct-ring-handler                  microservice.clj:   51

The confusing part is that (in the above example) AuthService does extend MicroService, just not the correct version of it.

The exception message should be extended to identify that this is "possibly because the protocol was reloaded since the class was defined."

A patch will be ready shortly.



 Comments   
Comment by Howard Lewis Ship [ 04/Aug/14 12:15 PM ]

Patch with tests





[CLJ-1473] Badly formed pre/post conditions silently passed Created: 24/Jul/14  Updated: 19/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: errormsgs, ft

Attachments: Text File 0001-Validate-that-pre-and-post-conditions-are-vectors.patch     Text File CLJ-1473_v02.patch     Text File CLJ-1473_v03.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Before:

user=> ((fn [x] {:pre (pos? x)} x) -5) ; ouch!
-5
user=> ((fn [x] {:pre [(pos? x)]} x) -5) ; meant this
AssertionError Assert failed: (pos? x)  user/eval4075/fn--4076 (form-init5464179453862723045.clj:1)

After:

user=> ((fn [x] {:pre (pos? x)} x) -5)
CompilerException java.lang.IllegalArgumentException: Pre and post conditions should be vectors, compiling:(NO_SOURCE_PATH:1:2) 
user=> ((fn [x] {:pre [(pos? x)]} x) -5)                                  
AssertionError Assert failed: (pos? x)  user/eval2/fn--3 (NO_SOURCE_FILE:2)
user=> ((fn [x] {:post (pos? x)} x) -5)
CompilerException java.lang.IllegalArgumentException: Pre and post conditions should be vectors, compiling:(NO_SOURCE_PATH:3:2) 
user=> ((fn [x] {:post [(pos? x)]} x) -5)              
AssertionError Assert failed: (pos? x)  user/eval7/fn--8 (NO_SOURCE_FILE:4)

Patch: CLJ-1473_v03.patch
Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 29/Apr/15 1:54 PM ]

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

Comment by Brandon Bloom [ 03/May/15 12:11 PM ]

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

Comment by Alex Miller [ 04/May/15 9:41 AM ]

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

Test should be improved as it could have caught that.

Comment by Brandon Bloom [ 04/May/15 7:25 PM ]

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





[CLJ-1448] Suggest alength in error message on attempt to access array length via .length Created: 19/Jun/14  Updated: 19/Jun/14  Resolved: 19/Jun/14

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

Type: Enhancement Priority: Trivial
Reporter: Colin Taylor Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: errormsgs

Attachments: Text File dot-length-recommend-alength.patch    
Patch: Code

 Description   

Problem:

Newcomers are easily confused by the inability to access <array>.length via (.length <array>).

Approach:

Append to invalid field access message, a suggestion to use alength for this specific case (class.isArray and field = "length")

user=> (.length (int-array 2))
IllegalArgumentException No matching field found: length for class [I, use alength function for array length


 Comments   
Comment by Alex Miller [ 19/Jun/14 2:56 PM ]

We don't have any plans to add anything this specific to the error check, thanks.





[CLJ-1436] Deref throws an unhelpful error message when used on something not dereferencable Created: 03/Jun/14  Updated: 26/Mar/15  Resolved: 25/Mar/15

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

Type: Enhancement Priority: Minor
Reporter: Phillip Lord Assignee: Unassigned
Resolution: Duplicate Votes: 3
Labels: errormsgs, newbie

Attachments: Text File deref.patch     File tests-patch.diff    
Patch: Code and Test

 Description   

Consider the following code:

(def x 1)
(def y (ref 2))

(+ @x y)

Clojure throws a ClassCastException on cast to Future. This is a very unhelpful error message; why a Future, why not Ref, Atom etc. It would be nice if this failed more gracefully.



 Comments   
Comment by Tobias Kortkamp [ 15/Sep/14 2:08 PM ]

Attached a patch with better error messages for deref. The above example now throws:

IllegalArgumentException class java.lang.Long is not derefable  clojure.core/deref (core.clj:2211)

and e.g.

(deref (delay 1) 500 :foo)

throws

IllegalArgumentException class clojure.lang.Delay is not derefable with a timeout  clojure.core/deref (core.clj:2222)
Comment by Mark Nutter [ 16/Sep/14 6:57 PM ]

Patch file clj-1436-patch-2014-09-16.diff updates the deref function so that it checks whether its arg is a future before sending it to deref-future. It also updates the deref function to provide clearer error messages. If the arg is not a future, and does not implement IDeref, the patched version of deref throws an IllegalArgumentException with a message that the arg cannot be dereferenced because it is not a ref/future/etc.

Comment by Mark Nutter [ 16/Sep/14 7:00 PM ]

Oops, I had this page open from yesterday and didn't see the patch submitted by Tobias. His has everything mine does, so I'll withdraw mine.

Comment by Mark Nutter [ 17/Sep/14 5:13 AM ]

One suggestion: the error message might sound better as "IllegalArgumentException cannot dereference clojure.lang.Delay; not a future or reference type".

Comment by Mark Nutter [ 17/Sep/14 5:44 AM ]

Tobias' patch does not contain the tests I had in mine, so I'm re-submitting just the tests as tests-patch.diff. If you install the tests patch without installing the deref patch, the tests will fail with the error message "Wrong exception type when passing non-IDeref/non-future to deref/@". Applying the deref patch as well will allow the tests to pass.

Comment by Alex Miller [ 28/Dec/14 11:06 AM ]

This patch does not seem to consider performance implications of adding this check and its impact on inlining.

Comment by Nicola Mometto [ 25/Mar/15 5:45 PM ]

Dupe of CLJ-1162

Comment by Andy Fingerhut [ 26/Mar/15 10:13 AM ]

Maybe it shouldn't be the deciding factor when selecting which duplicate ticket to close, but CLJ-1162 has 1 vote, this one has 3. Closing this one as the duplicate is discarding those votes, unless (a) they are copied over to the older ticket, or (b) consider closing the older ticket as a duplicate of this one instead.

Comment by Alex Miller [ 26/Mar/15 10:59 AM ]

I pondered that myself - I usually look at votes, current status, and patch. In this case, I liked the patch on CLJ-1162 better (although they were pretty similar).





[CLJ-1419] Report errors on missing param list or return type of methods in gen-class and gen-interface Created: 10/May/14  Updated: 12/May/14

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

Type: Enhancement Priority: Trivial
Reporter: Nathan Zadoks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, gen-class

Attachments: Text File 0001-CLJ-1419-default-to-void-return-type-in-gen-interfac.patch     Text File 0001-CLJ-1419-map-nil-to-void-in-prim-class.patch     File clj1419.clj     Text File fail.log    
Patch: Code

 Description   

The following are invalid and should produce errors when invoked on gen-class or gen-interface:

(gen-interface :name clj1419.IFail :methods [[myMethod java.lang.String]])  ;; no params, throws error
(gen-interface :name clj1419.IFail :methods [[myMethod []]]) ;; no return type
(gen-interface :name clj1419.IFail :methods [[myMethod]])  ;; no params or return type

The first example throws an error. The second and third do not but will generate an invalid class, verify with:

(.getMethods clj1419.IFail)
ClassNotFoundException java.lang.  java.net.URLClassLoader$1.run (URLClassLoader.java:366)

Add checks to prevent these errors.



 Comments   
Comment by Nathan Zadoks [ 10/May/14 1:34 PM ]

I've implemented both fixes, and attached them as patches.

Comment by Nathan Zadoks [ 10/May/14 1:40 PM ]

I'd argue that the behaviour of asm-type is at fault here (it can output an invalid type name when passed a nil argument), so I prefer that fix over the purely symptomatic generate-interface fix.

Comment by Andy Fingerhut [ 10/May/14 2:33 PM ]

Nathan, were you planning on submitting a signed Clojure Contributor's Agreement, or already have? Details here if you have not: http://clojure.org/contributing

Patches from non-contributors cannot be committed to Clojure.

Note: I cannot promise you that one of your patches will be accepted into Clojure if you sign a CA – only that it will not if you do not sign one.

Comment by Alex Miller [ 10/May/14 4:19 PM ]

Please add an example of how this happens and the current error.

Comment by Nathan Zadoks [ 11/May/14 3:45 AM ]

Andy — Yep, I've read up on that. My CA will be underway to Rich soon. (filled in, signed, in an envelope, just need to await the arrival of those bloody international stamps…)

Alex Miller — Tahdah!

A demonstration of the issue, both attached and as a gist: https://gist.github.com/nathan7/3a7e3a09e458f1354cbb

Comment by Nathan Zadoks [ 11/May/14 3:48 AM ]

and here's log of the compiler crash that results (also added to the gist now)

Comment by Nathan Zadoks [ 11/May/14 4:27 AM ]

Whoops, both of my patches were rather broken due to a misunderstanding on my side.
I forgot entirely that asm-type takes a symbol, not a string.
Modifying asm-type was definitely a bad idea, that check just looks whether it should defer to prim->class.
Adding nil to prim->class would work (and I've attached my patch for that too), but it's starting to look rather inelegant compared to just patching gen-interface.
(on a side note: I'm having a lot of fun exploring the Clojure codebase! thanks for that, humans!)

Comment by Alex Miller [ 11/May/14 7:26 AM ]

My reading of the docstring of gen-interface is that method declarations must specify a parameter list and a valid return type. I would expect all of these to be invalid:

(gen-interface :name clj1419.IFail :methods [[fail nil]])
(gen-interface :name clj1419.IFail :methods [[fail [] nil]])
(gen-interface :name clj1419.IFail :methods [[fail []]])

"nil" is not a valid type - you can use "void" for this purpose and this works fine:

(gen-interface :name clj1419.IFail :methods [[fail [] void]])

If this ticket is (as the title states) a request to allow omitting the return type or using "nil" as a return type, then I think the answer is no. If the ticket is a request to improve the error reporting of the failure cases above, then I think we can consider that but it will be very low priority.

Comment by Nathan Zadoks [ 12/May/14 8:19 AM ]

The code seems to suggest otherwise though, seeing the explicit extra branch for pclasses being nil.
As much as I like PL trivia, I haven't run into `void` in Clojure anywhere else yet, and I'm surprised to see it here.
Maintaining the principle of least surprise seems preferable to pedantry about whether nil is a type: (= "nil" (str (type (.methodReturningVoid obj)))

Comment by Alex Miller [ 12/May/14 8:26 AM ]

The two places to look for words to rely on are docstrings and the http://clojure.org/documentation pages. Implementation details are just that.

"nil" is not a type. "void" is a documented type identifier indicating the absence of a return value - http://clojure.org/java_interop#Java%20Interop-Aliases

Comment by Nathan Zadoks [ 12/May/14 8:27 AM ]

Okay. Better error-checking in asm-type then?

Comment by Alex Miller [ 12/May/14 8:49 AM ]

I have updated the title and description based on my understanding of what this ticket should be, which is enhanced error-checking on the method specs for gen-class and gen-interface. I'm not sure if that's in asm-type or somewhere earlier.





[CLJ-1401] CompilerException / IllegalStateException when overriding vars Created: 10/Apr/14  Updated: 31/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, errormsgs

Approval: Triaged

 Description   
=> (ns foo)
nil
=> (def a 1)
#'foo/a
=> (ns bar (:require [foo :refer :all]))
nil
=> (def a 2)
CompilerException java.lang.IllegalStateException: a already refers to: #'foo/a in namespace: bar, compiling:(NO_SOURCE_PATH:4:1)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6745)
	clojure.lang.Compiler.analyze (Compiler.java:6529)
	clojure.lang.Compiler.analyze (Compiler.java:6490)
	clojure.lang.Compiler.eval (Compiler.java:6801)
	clojure.lang.Compiler.eval (Compiler.java:6760)
	clojure.core/eval (core.clj:3079)
	clojure.main/repl/read-eval-print--7095/fn--7098 (main.clj:240)
	clojure.main/repl/read-eval-print--7095 (main.clj:240)
	clojure.main/repl/fn--7104 (main.clj:258)
	clojure.main/repl (main.clj:258)
	clojure.main/repl-opt (main.clj:324)
	clojure.main/main (main.clj:422)
Caused by:
IllegalStateException a already refers to: #'foo/a in namespace: bar
	clojure.lang.Namespace.warnOrFailOnReplace (Namespace.java:88)
	clojure.lang.Namespace.intern (Namespace.java:72)
	clojure.lang.Compiler$DefExpr$Parser.parse (Compiler.java:534)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6738)
	clojure.lang.Compiler.analyze (Compiler.java:6529)

I would expect (at worst) a similar warning to the initial namespace loading, rather than an exception here.



 Comments   
Comment by Alex Miller [ 11/Apr/14 8:26 AM ]

Could you put together a better reproducible test case for this that does not depend on core.matrix? Also, please include the (pst *e) when it occurs.

Comment by Andy Fingerhut [ 11/Apr/14 10:19 AM ]

I have tried the smallest possible Leiningen project I could think of that would cause the warnings about redefinitions, to see if I could get the exception to occur. 'lein new try1' to create the skeleton project, then edit src/try1/core.clj to contain only the following function definitions:

(defn merge
  "This definition of merge replaces clojure.core/merge"
  [x y]
  (- x y))

(defn *
  [x y]
  (* x y))

Then start a REPL with 'lein repl', and I see this behavior:

user=> (require '[try1.core :as c])
WARNING: merge already refers to: #'clojure.core/merge in namespace: try1.core, being replaced by: #'try1.core/merge
WARNING: * already refers to: #'clojure.core/* in namespace: try1.core, being replaced by: #'try1.core/*
nil
user=> (require '[try1.core :as c] )
nil
user=> (require '[try1.core :as c] :reload)
WARNING: merge already refers to: #'clojure.core/merge in namespace: try1.core, being replaced by: #'try1.core/merge
WARNING: * already refers to: #'clojure.core/* in namespace: try1.core, being replaced by: #'try1.core/*
nil

Ths all looks like behavior as I would expect, and I did not see the exception that Mike reports.

It seems that either Ctrl+Alt+L in Counterclockwise does something different than (require ... :reload), or there is something different about Mike's namespace in addition to redefining names in clojure.core that is causing the problem.

Comment by Alex Miller [ 11/Apr/14 11:17 AM ]

Marking this as NR for now - would be happy to see it reopened with an easily reproducible test case.

Comment by Mike Anderson [ 12/Apr/14 12:41 AM ]

To reproduce:

(ns op)
(defn * [a b] (clojure.core/* a b)) ;; gives warning
(ns use-op (:require [op :refer :all])) ;; gives warning
(ns use-op (:require [op :refer :all])) ;; gives error!

I believe Counterclockwise is simply loading the namespace again with CTRL-Alt+L, which is causing the ns form to be re-executed.

The docstring implies that ns can be used multiple times ("Sets ns to the namespace named by name (unevaluated), creating it if needed") so I would certainly expect multiple invocations of ns to be a no-op

Comment by Alex Miller [ 03/Nov/14 10:24 AM ]

Duped in CLJ-1578.

Comment by Mike Anderson [ 09/Jun/15 3:54 AM ]

This is still affecting me, and causing breakage with the latest versions of core.matrix. I don't know if this is a regression or not, but it certainly happens in 1.7.0-RC1

Any chance we can get a fix for 1.7? It is really annoying to have code fail because of this and force of refactoring of user code (my use case is adding a new var to clojure.core.matrix namespace, compiler error in user code that previously defined a var with the same name).

Comment by Mike Anderson [ 09/Jun/15 3:59 AM ]

Closing because I think this is better handled in the related issue

Comment by Mike Anderson [ 09/Jun/15 5:05 AM ]

Reopening because CLJ-1578 apparently does not resolve this specific issue, it only covers vars in clojure.core.

I'd still like to see this fixed for all namespaces, not just clojure.core.

Comment by Mike Anderson [ 09/Jun/15 5:08 AM ]

Reproduction:

=> (ns foo)
nil
=> (def a 1)
#'foo/a
=> (ns bar (:require [foo :refer :all]))
nil
=> (def a 2)
CompilerException java.lang.IllegalStateException: a already refers to: #'foo/a in namespace: bar, compiling:(NO_SOURCE_PATH:1:1)
=> clojure-version
{:major 1, :minor 7, :incremental 0, :qualifier "RC1"}

Stack trace:

CompilerException java.lang.RuntimeException: Unable to resolve symbol: pst in this context, compiling:(NO_SOURCE_PATH:1:1)
clojure.lang.Compiler.analyze (Compiler.java:6543)
clojure.lang.Compiler.analyze (Compiler.java:6485)
clojure.lang.Compiler$InvokeExpr.parse (Compiler.java:3737)
clojure.lang.Compiler.analyzeSeq (Compiler.java:6735)
clojure.lang.Compiler.analyze (Compiler.java:6524)
clojure.lang.Compiler.analyze (Compiler.java:6485)
clojure.lang.Compiler$BodyExpr$Parser.parse (Compiler.java:5861)
clojure.lang.Compiler$FnMethod.parse (Compiler.java:5296)
clojure.lang.Compiler$FnExpr.parse (Compiler.java:3925)
clojure.lang.Compiler.analyzeSeq (Compiler.java:6731)
clojure.lang.Compiler.analyze (Compiler.java:6524)
clojure.lang.Compiler.eval (Compiler.java:6789)
Caused by:
RuntimeException Unable to resolve symbol: pst in this context
clojure.lang.Util.runtimeException (Util.java:221)
clojure.lang.Compiler.resolveIn (Compiler.java:7029)
clojure.lang.Compiler.resolve (Compiler.java:6973)
clojure.lang.Compiler.analyzeSymbol (Compiler.java:6934)
clojure.lang.Compiler.analyze (Compiler.java:6506)
clojure.lang.Compiler.analyze (Compiler.java:6485)

Comment by Nicola Mometto [ 09/Jun/15 5:16 AM ]

As I already commented in CLJ-1578, I don't think this is a bug and I think this ticket should be declined.

Overriding non clojure.core vars has always (since 1.2 at least) caused an exception to be thrown.

Comment by Nicola Mometto [ 09/Jun/15 5:23 AM ]

Mike, maybe it would make sense to bring this issue up in the clojure-dev ml to get some opinions?

Comment by Mike Anderson [ 09/Jun/15 5:42 AM ]

Re-classify it as a feature request, if you prefer.

I still regard it as a defect because I expect :refer :all to work sanely.

Either way, this issue keeps breaking user code in my area (data science / exploratory statistics / data management). The ability to use / refer all is very useful for setting up a convenient namespace for exploratory work, so I don't accept that forcing users to explicitly require every single var used (as Nicola suggests in CLJ-1578) is a practical workaround.

I've also had it cause problems when working at the REPL and reloading namespaces.

If the Clojure core team really wants to keep this annoying behaviour, can we at least have some way to turn it off at the library level? Perhaps some namespace metadata that I can add to the clojure.core.matrix namespace to stop this from triggering?

Comment by Nicola Mometto [ 09/Jun/15 6:23 AM ]

Mike, this is just my personal opinion, I'm not a part of the core team and I don't speak for them, this is why I suggested you wrote on the clojure-dev ml.

Also to clarify, this issue you're reporting cannot manifest itself while reloading namespaces as the exception is thrown as soon as the redefinition happens.

Comment by Alex Miller [ 09/Jun/15 8:47 AM ]

You could use :exclude for this

(ns bar (:require [foo :refer :all :exclude (a)]))
Comment by Mike Anderson [ 09/Jun/15 9:30 AM ]

Hi Alex, that works as a fix when the problem occurs, but doesn't solve the problem of future user code breakage, unless the user accurately anticipates what symbols might get added to "bar" in the future. Which again seems like an unreasonable burden on the user.

What I'm arguing for, I guess, is a default presumption that if the user defines a var in their own namespace, they are happy to replace a similarly named var in any namespaces that they have previously use'd / refer-all'd.

If the user is genuinely concerned about overriding things by accident, I'd be happy with a warn-on-replace which does something analogous to warn-on-reflection. I proposed something similar in CLJ-1257 a while back, even wrote a patch that solves the whole problem in this way. Can we get that patch or something similar in 1.7?

Comment by Nicola Mometto [ 09/Jun/15 10:22 AM ]

CLJ-1746 is relevant and I like that proposal much better than CLJ-1257

Comment by Mike Anderson [ 09/Jun/15 10:43 AM ]

Hi Nicola, CLJ-1746 looks like a reasonable suggestion but still doesn't address the problem here, for the same reason that :exclude doesn't (see my comment to Alex)

The fundamental issue is that the current behaviour causes user code to break when libraries are upgraded to add new vars and requires changes to user code to fix / work around it. I consider that broken behaviour, or at least very bad design.

This is especially since we are encouraged to choose good names: I quote from the library coding standards(http://dev.clojure.org/display/community/Library+Coding+Standards)

"Use good names, and don't be afraid to collide with names in other namespaces. That's what the flexible namespace support is there for."

It isn't very flexible when user code keep breaking.....

Comment by Nicola Mometto [ 09/Jun/15 10:53 AM ]

From the same page, just a two lines below:
"Be explicit and minimalist about dependencies on other packages. (Prefer :require :refer in 1.4+ or :use :only in 1.0-1.3)."

If users carelessly import a whole package, I'd say that's their fault. Since clojure >1.3 the popular consensus has been that :use/:refer :all are not a good idea and people have been moving away from that, preferring :require :refer or :require :as instead.

The few that still use :use/:refer :all do it mostly for backward compatibility or in tests (I myself do it in tools.reader for the former reason).

I really don't think this is such a bad design choice as you seem to think, and I think that most people in the community would agree that the current behaviour and drive towards using :require :refer/require :as in lieu of :use/:refer :all is way more beneficial than harmful

Comment by Mike Anderson [ 09/Jun/15 11:42 AM ]

Hi Nicola, I have no issue with people using explicit :refer / :require, and I agree it is normal practice. It's good to be explicit and I generally do that myself (except in test / demo code).

However we aren't talking about that case : this issue is most relevant in those situations where users (for their own legitimate reasons) have decided to import a whole namespace. This is often convenient (e.g. REPL usage), sometimes it is valuable for specific purposes (e.g. testing).

I personally care about this mostly from the perspective of a library author: I want users to be able to use the library in whatever way is most convenient (which may include importing all vars), and I don't want user code to break randomly when I make a new release. Note that this also means I don't have control over user code so any solution that involves explicit requires, excludes, or some other manual workarounds is not a practical solution.

You can say "that's their fault" but this is currently supposed to be supported in Clojure so I think it ought to work sanely.

Comment by Alex Miller [ 09/Jun/15 12:00 PM ]

I think the library / evolution argument is a good one and I like that it reduces breakage due to evolution of 3rd party libraries. (I feel very strongly about this in clojure.core itself which is auto-referred, less strongly otherwise.)

On the flip side, if we remove this error, we should be explicit about what we are giving up to fully consider it. Presumably we are at least giving up a helpful error that occurs in the case of an accidental override. Are there other impacts?

I do not expect that we're going to change anything in 1.7.

Comment by Stuart Halloway [ 31/Jul/15 3:10 PM ]

Recategorizing as a feature request, as the current behavior was the original intent.

Could a namespace-reflecting macro provide for this use case without requiring a change to core?





[CLJ-1400] Error "Can't refer to qualified var that doesn't exist" should name the bad symbol Created: 09/Apr/14  Updated: 07/Oct/14

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: Compiler, errormsgs
Environment:

OS X


Attachments: File clj-1400-2.diff     File clj-1400-3.diff     File clj-1400-4.diff    
Patch: Code and Test
Approval: Incomplete

 Description   

Def of var with a ns that doesn't exist will yield this error:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Can't refer to qualified var that doesn't exist, compiling:(NO_SOURCE_PATH:1:1)

Cause: Compiler.lookupVar() returns null if the ns in a qualified var does not exist yet.

Proposed: The error message would be improved by naming the symbol and throwing a CompilerException with file/line/col info. It's not obvious, but this may be the only case where this error occurs. If so, the error message could be more specific that the ns is the part that doesn't exist.

Patch: clj-1400-4.diff

Screened by: Alex Miller



 Comments   
Comment by Scott Bale [ 25/Jun/14 9:58 AM ]

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

Comment by Scott Bale [ 26/Jun/14 11:23 PM ]

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

With this patch the example would now look like:

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

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

[footnote]

public static boolean namesStaticMember(Symbol sym){
	return sym.ns != null && namespaceFor(sym) == null;
}
Comment by Scott Bale [ 26/Jun/14 11:24 PM ]

patch: code and test

Comment by Scott Bale [ 26/Jun/14 11:27 PM ]

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

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)
Comment by Andy Fingerhut [ 29/Aug/14 4:46 PM ]

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

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

Comment by Scott Bale [ 31/Aug/14 3:53 PM ]

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

Comment by Alex Miller [ 09/Sep/14 9:29 AM ]

Few comments to address:

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

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

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

(properly named patch)

Comment by Alex Miller [ 11/Sep/14 11:37 PM ]

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

Comment by Scott Bale [ 19/Sep/14 2:37 PM ]

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

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

In the Repl...

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

...or in a source file

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

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

Comment by Alex Miller [ 07/Oct/14 12:34 PM ]

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





[CLJ-1394] Print multi method dispatch values in the exception messages. Created: 31/Mar/14  Updated: 01/Apr/14

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

Type: Enhancement Priority: Trivial
Reporter: Roman Scherer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

Attachments: File pr-str-dispatch-value-safe.diff    
Patch: Code and Test

 Description   

The error messages of defmulti are at the moment not as helpful as they could be under certain circumstances. Calling this multi method with a lazy seq as it's dispatch argument raises the following exception:

(defmulti test-multi identity)
(defmethod test-multi 1 [arg] true)

(test-multi (map identity [:x]))
;=> java.lang.IllegalArgumentException: No method in multimethod 'test-multi' for dispatch value: clojure.lang.LazySeq@3c6f1187

Sometimes it would be useful to actually see which values are in the lazy seq being dispatched on. A better error message could look like
this for example:

(test-multi (map identity [:x]))
;=> java.lang.IllegalArgumentException: No method in multimethod 'test-multi' for dispatch value (:x) of class clojure.lang.LazySeq

This patch addresses this issue by formatting the dispatch value via `pr-str` and printing the class before it is passed to the exception constructor. The same is also done for the methods in MultiFn.java that throw a dispatch value as part of their exception message.



 Comments   
Comment by Nicola Mometto [ 31/Mar/14 8:22 PM ]

What if the value is infinite lazy-seq?

Comment by Roman Scherer [ 01/Apr/14 2:50 AM ]

Nicola, I forgot those. But I think infinite sequences could be handled with:

(set! *print-length* 10)

I'll try it out and will update the patch later.

Any other edge cases in mind?

Comment by Roman Scherer [ 01/Apr/14 2:28 PM ]

After having read "Controlling run-away trains, onions, and exercise
bikes" [1] I now bind print-length and print-size when building
the error message. This helps when not being able to dispatch on this
for example:

(test-multi (let [x (atom 0)] (reset! x {:deeper x})))

However I'm not sure if this helps in the following case, where
dispatching would fail on an infinite seq.

(test-multi (iterate inc 0))

The above doesn't terminate in Clojure 1.6.0, nor does it when binding
print-length like the attached patch does.

[1] http://blog.n01se.net/blog-n01se-net-p-85.html





[CLJ-1341] keyword function returns nil on bad input Created: 05/Feb/14  Updated: 24/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: checkargs, errormsgs

Attachments: Text File keyword-1341-2014-02-12.2.patch     Text File keyword-1341-2014-02-12.patch    
Patch: Code and Test

 Description   

The keyword function should throw an exception on bad input rather than return nil.

user=> (keyword 5)
nil
user=> (keyword [])
nil

Cause: The keyword function is defined as a cond with cases for keywords, symbols, and strings. There is no :else so all other cases return nil.

Proposal: Add an :else branch and throw an exception in keyword.

Patch:



 Comments   
Comment by Eric Normand [ 12/Feb/14 7:17 PM ]

The keyword function should throw an IllegalArgumentException on wrong argument type rather than return nil. For consistency, the two-argument case should throw an IllegalArgumentException if not both arguments are strings.

The find-keyword function should behave similarly to maintain the same signature.

Current behavior:

user=> (keyword 5)
nil
user=> (keyword [])
nil
user=> (keyword 1 1)
java.lang.ClassCastException: java.lang.Long cannot be cast to java.lang.String

Cause: The keyword function is defined as a cond with cases for keywords, symbols, and strings. There is no :else so all other cases return nil. The same goes for find-keyword, which should behave in the same way. The two-argument case does throw an exception, but the message is not very helpful.

Proposal: I have added an :else branch to the cond that throws an IllegalArgumentException with a message that indicates the acceptable types and prints the actual argument. I made the same change to find-keyword. There are also simple tests.

Patch: keyword-1341-2014-02-12.patch

Note: This change does not check for all bad input, just the type. For instance, it is still possible to pass in a string with "illegal" keyword characters.

Comment by Alex Miller [ 12/Feb/14 9:20 PM ]

Hey Eric, thanks for the patch! The 1 arg change looks good.

On the 2 arg change I have a concern - I'm worried that we are adding new checks into a pretty hot code path (keyword creation). The 2 arg path is not a silent failure as you'll get a ClassCastException so I do not think adding these checks here is worth it. In the 1-arg case you've already fallen through the else, so there's no additional cost.

Comment by Eric Normand [ 12/Feb/14 9:35 PM ]

Understood. I'll remove the two-argument case.

Comment by Eric Normand [ 12/Feb/14 9:51 PM ]

The keyword function should throw an IllegalArgumentException on wrong argument type rather than return nil. The two-argument case already throws an exception.

The find-keyword function should behave similarly to maintain the same signature.

Current behavior:

user=> (keyword 5)
nil
user=> (keyword [])
nil
user=> (keyword 1 1)
java.lang.ClassCastException: java.lang.Long cannot be cast to java.lang.String

Cause: The keyword function is defined as a cond with cases for keywords, symbols, and strings. There is no :else so all other cases return nil. The same goes for find-keyword, which should behave in the same way.

Proposal: I have added an :else branch to the cond that throws an IllegalArgumentException with a message that indicates the acceptable types and prints the actual argument. I made the same change to find-keyword. There are also simple tests.

Alternatives: Adding checks for the two-argument case was considered but it was feared that adding the extra overhead was not worth it since it already threw an exception. No significant overhead is added in the single-argument case since it will only affect erroneous input.

Patch: keyword-1341-2014-02-12.2.patch

Note: This change does not check for all bad input, just the type. For instance, it is still possible to pass in a string with "illegal" keyword characters.





[CLJ-1326] Inconsistent reflection warnings when target is a literal Created: 19/Jan/14  Updated: 05/Feb/14

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

Type: Enhancement Priority: Trivial
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs


 Description   
user=> (set! *warn-on-reflection* true)
true
user=> (.get {} 0)
Reflection warning, NO_SOURCE_PATH:2:1 - call to get can't be resolved.
nil
user=> (.get {1 1} 0)
nil
user=> (.get ^:foo {1 1} 0)
Reflection warning, NO_SOURCE_PATH:4:1 - call to get can't be resolved.
nil
user=> (.get {1 (inc 0)} 0)
Reflection warning, NO_SOURCE_PATH:5:1 - call to get can't be resolved.
nil

Similar issues apply to other literals (vector literals, list literals)






[CLJ-1325] Report warnings if *unchecked-math* and boxing happens Created: 16/Jan/14  Updated: 11/Nov/14  Resolved: 29/Aug/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 1
Labels: errormsgs, math

Attachments: File boxed.diff     Text File boxedmath.txt     Text File clj-1325.patch     Text File clj-1325-v2.patch     Text File clj-1325-v3.patch    
Patch: Code and Test
Approval: Ok

 Description   

Currently, it is difficult to tell that the compiler is using boxed math unless you look at the generated bytecode. The proposed enhancement here is to emit new warnings if *unchecked-math* is on and boxed math is occurring.

Approach: In the compiler, when compiling a StaticMethodExpr, if *unchecked-math* is true and the class is clojure.lang.Numbers and one of the parameters of static method is of type java.lang.Object or java.lang.Number, then emit a warning at compile-time.

In addition, there is a new WarnBoxedMath Java annotation - a small number of methods on Numbers with Object parameters use this annotation to indicate that warning should not take place. The same annotation can be (but is not currently) used to mark methods on Numbers without Object/Number params that should warn. See boxedmath.txt for a list of methods and categories.

Patch: clj-1325-v3.patch

Screened by:



 Comments   
Comment by Alex Miller [ 14/Apr/14 10:56 PM ]

Moving to 1.7.

Comment by Alex Miller [ 15/Apr/14 10:17 AM ]

List of methods in Numbers and whether they should be considered "boxed math" or not, with some questions.

Comment by Alex Miller [ 14/May/14 2:34 PM ]

Ready for screening.

Comment by Alex Miller [ 16/May/14 11:19 AM ]

clj-1325-v2.patch is identical to last except for a cleaned up the commit message.

Comment by Alex Miller [ 16/May/14 11:51 AM ]

Added v3 patch that just reworks block/indentation style to match surrounding code better.

Comment by Stuart Sierra [ 16/May/14 1:15 PM ]

Screened. Comments:

1) There is no way to get both overflow checks and boxed-math warnings at the same time. Maybe this doesn't matter.

2) The error messages aren't ideal, because they refer to clojure.lang.Numbers, but we can assume that anyone savvy enough to be using *unboxed-math* will also be savvy enough to know what clojure.lang.Numbers is.

3) This doesn't protect me from autoboxing in arbitrary Java method calls, but normal reflection warnings should catch most real-world cases, since few Java APIs overload on primitive and Object.

Comment by Nicola Mometto [ 11/Nov/14 6:02 PM ]

With the new :warn-on-boxed, this code reports a warning:

user=> (defn f [x] (inc x))
Boxed math warning, NO_SOURCE_PATH:2:13 - call: public static java.lang.Number clojure.lang.Numbers.unchecked_in
#'user/f

but this does not:

user=> (defn f1 [^long x] (inc x))
#'user/f1

is this intentional?

Comment by Alex Miller [ 11/Nov/14 9:00 PM ]

The bytecode for those methods is:

f:
  public java.lang.Object invoke(java.lang.Object);
    Code:
       0: aload_1
       1: aconst_null
       2: astore_1
       3: invokestatic  #34                 // Method clojure/lang/Numbers.unchecked_inc:(Ljava/lang/Object;)Ljava/lang/Number;
       6: areturn

f1: 
  public final java.lang.Object invokePrim(long);
    Code:
       0: lload_1
       1: invokestatic  #36                 // Method clojure/lang/Numbers.unchecked_inc:(J)J
       4: invokestatic  #40                 // Method clojure/lang/Numbers.num:(J)Ljava/lang/Number;
       7: areturn

I assume your question is why the call to Numbers.num(long) at the end doesn't cause the warning due to the return type? I had those num() calls in my early list of questionables. This function is tricky because it's called from lots of other methods (many of which already trigger the warning), so it has the potential to cause multiple warnings on a single expression. But this does indeed seem like a common and important case to suggest a return type hint.

Any of these calls that take prims but return a Number or Object require a judgement call and an explicit annotation - there is certainly room for interpretation on some of them.

Adding the return type hint cleans things up pretty well:

public final long invokePrim(long);
    Code:
       0: lload_1
       1: lconst_1
       2: ladd
       3: lreturn
Comment by Alex Miller [ 11/Nov/14 9:06 PM ]

I created CLJ-1585 for this.





[CLJ-1319] array-map fails lazily if passed an odd number of arguments Created: 08/Jan/14  Updated: 03/Aug/15

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

Type: Defect Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, ft

Attachments: Text File 0001-CLJ-1319-Throw-on-odd-arguments-to-PersistentArrayMa.patch     Text File 0002-CLJ-1319-Throw-on-odd-arguments-to-PersistentArrayMa.patch    
Patch: Code and Test
Approval: Ok

 Description   

If called with an odd number of arguments, array-map does not throw an exception until the map is realized, when it throws the confusing ArrayIndexOutOfBoundsException.

Example, in 1.5.1 and 1.6.0-alpha3:

user=> (def m (hash-map :a 1 :b))
IllegalArgumentException No value supplied for key: :b  clojure.lang.PersistentHashMap.create (PersistentHashMap.java:77)

user=> (def m (array-map :a 1 :b))
#'user/m
user=> (prn m)
ArrayIndexOutOfBoundsException 3
  clojure.lang.PersistentArrayMap$Seq.first
  (PersistentArrayMap.java:313)

Approach: Catch on construction and throw same error as hash-map.

user=> (def m (array-map :a 1 :b))
IllegalArgumentException No value supplied for key: :b  clojure.lang.PersistentArrayMap.createAsIfByAssoc (PersistentArrayMap.java:78)

Patch: 0002-CLJ-1319-Throw-on-odd-arguments-to-PersistentArrayMa.patch
Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 08/Jan/14 11:01 AM ]

PersistentArrayMap.createAsIfByAssoc could check length is even to catch this

Comment by Jason Felice [ 27/Jan/14 1:01 PM ]

A better error message would be nice... this is the best I could think of.

Comment by OHTA Shogo [ 14/May/15 7:06 AM ]

I suffered from this problem recently. Anything to resolve before applying the patch?

Comment by Alex Miller [ 14/May/15 8:21 AM ]

The error message should match the message for hash-map:

user=> (hash-map 1 2 3)
IllegalArgumentException No value supplied for key: 3  clojure.lang.PersistentHashMap.create (PersistentHashMap.java:77)
user=> (array-map 1 2 3)
IllegalArgumentException Odd number of arguments when creating map  clojure.lang.PersistentArrayMap.createAsIfByAssoc (PersistentArrayMap.java:78)
Comment by OHTA Shogo [ 14/May/15 8:49 PM ]

Let me have a try. I just updated the patch!

0002-CLJ-1319-Throw-on-odd-arguments-to-PersistentArrayMa.patch

Comment by Alex Miller [ 15/May/15 9:10 AM ]

Looks good.

Comment by Alex Miller [ 03/Aug/15 8:05 AM ]

Reopened - does not seem to have correct behavior in 1.8.0-alpha4.

user=> (array-map 1 2 3)
ArrayIndexOutOfBoundsException 3  clojure.lang.PersistentArrayMap$Seq.first (PersistentArrayMap.java:321)
user=> (def m (array-map :a 1 :b))
#'user/m
Comment by Alex Miller [ 03/Aug/15 8:17 AM ]

Doesn't like this was merged so moving to Ok but not closed for next round.





[CLJ-1309] Bindings after :as in list destructuring should throw error Created: 19/Dec/13  Updated: 05/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: ben wolfson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: Compiler, errormsgs


 Description   

If you try to define a vector binding with anything at all after an :as parameter, you do not get a compiler error, and the binding is silently swallowed:

user> ((fn [[:as y z]] y) [1 2])
[1 2]

If you try to actually use the binding, there will be a compiler error (the compiler will complain that there's no binding for the symbol), but the actual error has already happened, and should be reported earlier.






[CLJ-1297] try to catch using - instead of _ in filenames so the compiler can give a better error message for people who don't know that you need to use _ in file names Created: 19/Nov/13  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Completed Votes: 11
Labels: compiler, errormsgs

Attachments: File better-error-messages-for-require.diff     Text File clj-1297-v3.patch     Text File clj-1297-v5.patch    
Patch: Code
Approval: Ok

 Description   

Problem: Clojure requires the files that back a namespace that has dashes in it to have the dashes replaced with underscores on the filesystem (ie a.b_c.clj for namespace a.b-c). If you require a file that has been mistakenly saved as b-c.clj instead, you will get an error message:

Exception in thread "main" java.io.FileNotFoundException: Could not locate a/b_c__init.class or a/b_c.clj on classpath:
...

Proposed:
Fix the bad ending colon in this sentence and add a second sentence only when the file name has an _ in it: "Please check that namespaces with dashes use underscores in the Clojure file name."

Patch: clj-1297-v5.patch

Screened by: Alex Miller



 Comments   
Comment by Joshua Ballanco [ 20/Nov/13 12:15 AM ]

A perhaps even better solution would be to simply allow the use of dashes in *.clj[s] filenames. I can't imagine the extra disk access per-namespace would be a huge performance burden, and (since dashes aren't allowed currently) I don't think there would be any issues with backwards compatibility.

Comment by Gary Fredericks [ 20/Nov/13 8:40 AM ]

It's worth mentioning the combinatorial explosion for namespaces with multiple dashes – if I (require 'foo-bar.baz-bang), should clojure search for all four possible filenames? Does the jvm have a way to search for files by regex or similar to avoid nasty degenerate cases (like (require 'foo-------------))?

Comment by Joshua Ballanco [ 20/Nov/13 11:08 AM ]

According to the docs, the FileSystem class's "getPathMatcher" method accepts path globs, so you'd merely have to replace each instance of "-" or "_" with "{-,_}". Actual runtime characteristics would likely depend on the underlying filesystem's implementation.

Comment by Alex Miller [ 20/Nov/13 12:02 PM ]

I don't think the FileSystem stuff applies when looking up classes on the classpath. Note that Java class names cannot contain "-".

Comment by Phil Hagelberg [ 21/Nov/13 12:05 PM ]

According to the spec, Java class names can't contain dashes (though IIRC OpenJDK and Oracle's JDK accept them anyway) but the requirement that Clojure source files have names which align with their AOT'd class file eqivalents is something we've imposed upon ourselves. Introducing the disconnect between .clj files and .class files makes way more sense than disconnecting namespaces and .clj files, but arguably it's too late to fix that mistake.

In any case a check for dashed files (resulting only in a more informative compiler error, not a more permissive compiler) which only triggers when a .clj file cannot be found imposes zero overhead in the case where things are already working.

Comment by scott tudd [ 09/Dec/13 2:19 PM ]

As Clojure seems to be idiomatic to have sometimes-dashed-namespace-and-function-names as opposed to the ubiquitous camelCaseFunctionNames in java ... I agree to have the compiler automagically handle 'knowing' to look in dir_struct AND dir-struct for requisite files.

or at the least print out a nice message explaining the quirk when files "can't" be found ... WHEN there are dashes and underscores involved... anything to aid in helping things "just work" as one would think they're supposed to.

Comment by Obadz [ 12/Dec/13 5:28 AM ]

I would have saved a few hours as well.

Comment by Alexander Redington [ 14/Feb/14 2:29 PM ]

This patch changes clojure.core/load such that:

  • When loading the resource-root of lib throws a FileNotFoundException, the lib is analyzed...
  • ... if the lib was a name that would be munged, it examines the combinatorial explosion of munge candidates and .clj or .class files in the classpath ...
  • ... if any of these candidates exist, it informs the user of the file's existance, and that a change to that filename would lead to that resource being loaded.
  • ... if none of these candidates exist, it throws the original exception.

It also modifies clojure.lang.RT to expose the behavior around finding clj or class files from a resource root.

Comment by Andy Fingerhut [ 20/Mar/14 1:16 PM ]

I do not know whether it handles all of the cases proposed in this discussion, but I encourage folks to check out the filename/namespace consistency checking in the latest Eastwood release (version 0.1.1) to see if it catches the cases they would hope to catch. It does a static check based on the files in a Leiningen project, nothing at run time. https://github.com/jonase/eastwood

Of course changes to Clojure itself to give warnings about such things can still be very useful, since not everyone will be using a 3rd party tool to check for such things.

Comment by Alex Miller [ 27/Jun/14 2:24 PM ]

Re the screener's note at the top, my preference would be for the simpler approach.

Comment by Rich Hickey [ 29/Aug/14 9:48 AM ]

I see no reason to fish around in the file system at all. Why can't the message simply remind people that underscores are required and to check that they aren't using dashes?

Comment by Gary Fredericks [ 30/Sep/14 4:43 PM ]

The tradeoff is that fishing around in the file system means the error message is only shown if the user likely made the relevant mistake, whereas simply showing the error message would (if I understand correctly) mean this reminder gets shown for every require error, which I would estimate happens a whole lot more often than the mistake in question.

Comment by Andy Fingerhut [ 30/Sep/14 5:05 PM ]

Gary, there is an exception thrown in any case if the load fails. One approach that I am hacking up now is to add to the existing exception's message that maybe they need to replace - chars in file name with _, but only if the name attempted to be loaded has at least one '-' character in it. So no new output, except in an exception already being thrown, and then only if there is at least a possibility that this is the problem.

Comment by Andy Fingerhut [ 30/Sep/14 5:10 PM ]

clj-1297-v2.patch is similar to the previously proposed patch, but does not touch the file system in any way that wasn't already being done before this patch.

It adds an extra hint to the message of the exception already being thrown if the resource is not found, but only if there is a '-' character in the name that failed to load.

Comment by Andy Fingerhut [ 30/Sep/14 6:45 PM ]

clj-1297-v3.patch is nearly identical to clj-1297-v2.patch, described in the previous comment, except it eliminates an unnecessary let expression.

Comment by Alex Miller [ 01/Oct/14 3:05 PM ]

With latest:

user=> (require 'a-b)
FileNotFoundException Could not locate a_b__init.class or a_b.clj on classpath:   Perhaps a file you are attempting to load needs - chars in name replaced with _  clojure.core/load-one/fn--5135 (core.clj:5606)

That looks goofy due to the base message in RT.load(): throw new FileNotFoundException(String.format("Could not locate %s or %s on classpath: ", classfile, cljfile));

I would like to:
1) Fix that RT.load() message to not end with a colon: "Could not locate %s or %s on classpath."
2) Instead of changing load-one, just add the additional sentence in RT.load(). Second optional sentence could be: "Please check that namespaces with dashes use underscores in the Clojure file name."

Final message would then look like:

"Could not locate a_b__init.class or a_b.clj on classpath. Please check that namespaces with dashes use underscores in the Clojure file name."

Comment by Andy Fingerhut [ 02/Oct/14 12:06 AM ]

Patch clj-1297-v4.patch modifies only RT.load, including an extra message only if the file name in the argument contains a '_' character in it, with the message suggested by Alex Miller in his last comment before this.

Comment by Alex Miller [ 02/Oct/14 8:55 AM ]

The bad end colon is not fixed and the optional message is not included in the format string.

Comment by Andy Fingerhut [ 02/Oct/14 11:04 AM ]

clj-1297-v5.patch should be the one. I can only attempt to blame the previous bone-headed failure on lack of sleep, but even then ...





[CLJ-1284] Clojure functions and reified objects should expose a public static field to identify their proper Clojure name Created: 24/Oct/13  Updated: 29/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs

Attachments: Text File CLJ-1284-store-demunged-names.patch    

 Description   

There are several examples of frameworks that attempt to de-mangle a Java class name into a Clojure symbol (including namespace); this is useful for writing out an improved, Clojure-specific stack trace when reporting exceptions.

Existing libraries are based on regular expression matching and guesswork, and can occasionally give incorrect results, such as when a namespace or function name actually contains an underscore.

It would be helpful for authors of such frameworks if Clojure would expose a static final field on such classes with the proper name that should appear in the stack trace; libraries would then be able to use reflection to access the proper name of the field, without the current guesswork.

I would suggest CLOJURE_SOURCE_NAME as a reasonable name for such a field.

Other Clojure class constructs beyond functions, such as reified types and protocol implementations, would also benefit, though it is less obvious what exact string value would properly and unambiguously identify what purpose the class plays.



 Comments   
Comment by Alex Miller [ 24/Oct/13 8:31 PM ]

FYI, there is a patch on the way in for 1.6 that contains a new demunge function in Compiler. However, the munged name is not always reversible so having the original around is a good idea.

Comment by Andy Fingerhut [ 25/Oct/13 11:10 AM ]

The patch Alex is referring to is attached to CLJ-1083.

Comment by Andy Fingerhut [ 25/Oct/13 11:13 AM ]

Howard, there seems to be some overlap in the intent between this ticket and CLJ-1278. I guess either of them could be done without the other, but wanted to check.

Comment by Daniel Solano Gómez [ 20/Aug/14 2:17 PM ]

Here's an initial stab at adding this feature.

Some notes:

  • This will tag emitted classes from deftype and fn
  • This will handle fn}}s that are enclosed, but the output will be slightly different from the standard {{demunge function: only the initial $ is transformed to a /.
  • Unfortunately, because the defn for type/record constructor occurs in a let form, the generated symbol doesn't match what it should be.
  • There is no exposed API to get the demunged symbol from the class. Perhaps demunge should check if the given name corresponds to a class with this field?

I welcome any input on how this should really work. In particular, any ideas on how to best deal with {{defn}}s that are not top-level forms.

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

Patch CLJ-1284-store-demunged-names.patch dated Aug 20 2014 does not apply cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. I have not checked whether it applied cleanly before that day, nor have I checked how easy or difficult it might be to update this patch.





[CLJ-1282] The quote special form should throw an exception if passed more than one form to quote Created: 23/Oct/13  Updated: 13/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: errormsgs, ft, reader

Attachments: Text File CLJ-1282-p1.patch     Text File CLJ-1282-p2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Quote currently ignores all but the first argument. In the case of being called accidentally with multiple values, it should throw an exception specifying the error.

user> (quote 1 2 3)
1

Patch: CLJ-1282-p2.patch

Screened by: Alex Miller

------- Original: --------

Every once in a while, you can just go down the rabbit hole.

I had an errant expression in my code:

(-> message get-message-values 'DESTINATION_MERCHANT_ID)

One would think this would work; it certainly would if the key was a keyword and not a symbol.

One would expect this to expand to:

('DESTINATION_MERCHANT_ID (get-message-values message))

however, the reader is involved, so it is as if the source were:

(-> message get-message-values (quote DESTINATION_MERCHANT_ID))

which expands to:

(quote (-> message get-message-values) DESTINATION_MERCHANT_ID))

... hilarity ensues! Because quote currently ignores extra parameters, my code gets the quoted value '(clojure.core/-> message get-message-values) rather than the expected string from the map; this shifts us from the "there's a bug in my code" to "the nature of reality is broken".

The correct expression is:

(-> message get-message-values (get 'DESTINATION_MERCHANT_ID))

This took quite a while to track down; if the

special form checked that it was passed exactly one form to quote and threw an exception otherwise, I think I would have caught this much earlier. It could even identify the expression it is quoting, which would provide a lot better understanding of where I went wrong.



 Comments   
Comment by Howard Lewis Ship [ 23/Oct/13 2:02 PM ]

Sorry, can't edit the description now to correct the formatting errors.

Comment by Howard Lewis Ship [ 24/Oct/13 6:09 PM ]

I just wanted to point out that your description is shorter, but makes it appear that such a use is unlikely and therefore unimportant; the detail of my description is to point out a reasonable situation where something explicable, but completely counterintuitive and confusing, does occur.

Comment by Alex Miller [ 24/Oct/13 8:28 PM ]

That's why I left the original in there too.

Comment by Gary Fredericks [ 09/Dec/13 7:07 AM ]

(quote) currently returns nil. Do we have an opinion about that?

Comment by Gary Fredericks [ 09/Dec/13 9:32 AM ]

Attached p1, which throws an IllegalArgumentException (wrapped in a CompilerException of course) for anything but 1 arg, and includes the number of args that were passed.

I can't think of any reason why (quote) would be useful, so I decided to throw on that too. Very easy to change of course.

Also added a test that (eval '(quote 1 2 3)) throws.

Comment by Stuart Halloway [ 31/Jan/14 12:46 PM ]

I recommend the following changes:

  • throw an ex-info that includes the offending form in its map {:form ...}
  • check only for the map data, not exception type or message, in the tests
Comment by Andy Fingerhut [ 31/Jan/14 6:31 PM ]

Patch CLJ-1282-p1.patch no longer applies cleanly after commits made to Clojure master on Jan 31 2014, probably due to the patch committed for CLJ-1318, and probably only because some lines of context changed in the test file. That would be trivial to update, but Stu's comments above suggest that more significant changes need to be made.

Comment by Gary Fredericks [ 01/Feb/14 9:19 AM ]

Throwing an ex-info is easy enough. I don't know how to avoid at least incidentally checking for the exception type, since the ExceptionInfo is wrapped in a CompilerException. I'll make a patch that keeps the class name in the test but doesn't do any checks on the cause aside from the ex-data. Let me know if I should do anything different.

Comment by Gary Fredericks [ 01/Feb/14 9:58 AM ]

Attached CLJ-1282-p2.patch which is off of the current master and addresses Stu's points.

Comment by Alex Miller [ 04/Feb/14 11:23 PM ]

Moving back to Triaged for more looks.

Comment by Nicola Mometto [ 16/Feb/14 12:12 PM ]

Currently (quote) returns nil, is it intended that this patch makes that an error or was this by accident?

Comment by Gary Fredericks [ 16/Feb/14 12:23 PM ]

I consciously chose to make (quote) an error – I made a comment about that earlier and didn't get any feedback, so I unilaterally decided to make it an error due to the fact that I couldn't think of any possible use for (quote).

It's an easy switch if somebody thinks differently.

Comment by Nicola Mometto [ 16/Feb/14 1:13 PM ]

I'm sorry I did not notice your previois comment.
I'm asking because I need to know whether I should throw on (quote) for tools.analyzer, currently it is allowed but I too think that (quote) should be an error.

Comment by Alex Miller [ 29/Apr/15 11:51 AM ]

I also think (quote) should throw an error.

Comment by Stuart Halloway [ 13/Jul/15 5:35 PM ]

This is an API change request, not a defect, and the change recommended here could break (admittedly ill-advised) programs.





[CLJ-1279] Fix confusing macroexpand1 ArityException handling Created: 16/Oct/13  Updated: 05/Feb/14

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

Type: Defect Priority: Major
Reporter: Alex Coventry Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: Compiler, errormsgs, macro

Attachments: Text File 0001-Edit-macro-ArityException-in-AFn.patch     Text File 0001-Fix-macroexpand1-s-handling-of-ArityException.patch    
Patch: Code and Test

 Description   

macros can give very confusing error messages when they execute a form which generates an ArityException. clojure.lang.Compiler.macroexpand1 assumes that any ArityException comes from the call to the macro itself, which need not be the case. For instance:

user> (do (defmacro f [] (assoc)) (f))
ArityException Wrong number of args (-2) passed to: core$assoc clojure.lang.Compiler.macroexpand1 (Compiler.java:6488)
user> (use 'clojure.repl) (pst)
nil
ArityException Wrong number of args (-2) passed to: core$assoc
clojure.lang.Compiler.macroexpand1 (Compiler.java:6488)
clojure.lang.Compiler.macroexpand (Compiler.java:6544)
clojure.lang.Compiler.eval (Compiler.java:6618)
clojure.lang.Compiler.eval (Compiler.java:6624)
clojure.lang.Compiler.eval (Compiler.java:6597)
clojure.core/eval (core.clj:2864)
clojure.main/repl/read-eval-print-6596/fn-6599 (main.clj:260)
clojure.main/repl/read-eval-print--6596 (main.clj:260)
clojure.main/repl/fn--6605 (main.clj:278)
clojure.main/repl (main.clj:278)
clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn--1251 (interruptible_eval.clj:56)
clojure.core/apply (core.clj:617)
nil

Easy enough to see the source of the problem in this case, but because both the number of arguments actually passed is off by two, and the stacktrace element for the call to assoc has been dropped, this shortcut by macroexpand1 can get super confusing.

The attached patch corrects this behavior. E.g.

user=> (do (defmacro f [] (assoc)) (f))
ArityException Wrong number of args (0) passed to: core$assoc clojure.lang.AFn.throwArity (AFn.java:437)
user=> (use 'clojure.repl) (pst)
nil
ArityException Wrong number of args (0) passed to: core$assoc
user/f (NO_SOURCE_FILE:1)
clojure.lang.Var.invoke (Var.java:419)
clojure.lang.Var.applyTo (Var.java:532)
clojure.lang.Compiler.macroexpand1 (Compiler.java:6507)
clojure.lang.Compiler.macroexpand (Compiler.java:6580)
clojure.lang.Compiler.eval (Compiler.java:6654)
clojure.lang.Compiler.eval (Compiler.java:6660)
clojure.lang.Compiler.eval (Compiler.java:6633)
clojure.core/eval (core.clj:2864)
clojure.main/repl/read-eval-print-6594/fn-6597 (main.clj:260)
clojure.main/repl/read-eval-print--6594 (main.clj:260)
clojure.main/repl/fn--6603 (main.clj:278)
nil



 Comments   
Comment by Alex Coventry [ 17/Oct/13 11:01 AM ]

Patch with test

Comment by Alex Coventry [ 23/Oct/13 11:42 PM ]

Amended patch to deal more gracefully with unexpected stack trace structure.

Comment by Alex Miller [ 24/Oct/13 12:09 AM ]

Also see CLJ-397 and CLJ-383.

Comment by Alex Coventry [ 24/Oct/13 2:46 PM ]

Thanks, Alex. It would be easy enough to move most of the logic into ArityException, which would be a compromise between Stu's[1] options 1 and 2. Is that worth doing?

Amending clojure.lang.AFn.throwArity to check whether "this" is a macro and adjust the arg count there accordingly might be the simplest way. I can see why Rich prefers all the logic to go into ArityException, but since ArityExceptions are used for things other than macros, I don't see a way to make an honest error message there without groveling the stack trace.

[1] http://dev.clojure.org/jira/browse/CLJ-397?focusedCommentId=24090&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-24090

Comment by Alex Miller [ 24/Oct/13 2:57 PM ]

I would have to take more time than I have to make an informed opinion but I can say that from a general point of view inspecting StackTraceElements does not seem like the right solution to (almost) any problem.

Comment by Alex Coventry [ 24/Oct/13 10:26 PM ]

This patch causes Var.setMacro to set instance attribute AFn.macrop to true, so that AFn.throwArity can reduce the number of arguments reported.

I'm not used to negotiating java class hierarchies, so it's possible there's a cleaner way. Since Var.fn() returns an IFn, I added macrop handling methods IFn.setMacro and IFn.isMacro. These then needed to be implemented in Ref and Keyword, as well as AFn (where I wanted them) because they implement the IFn interface but don't inherit from AFn.

The real drawback I see with this approach is the duplicated state, though: ^{:macro true} vs AFn.macrop==true.

Comment by Andy Fingerhut [ 25/Oct/13 6:33 PM ]

I have not investigated the reason yet, but neither patch applies cleanly after the latest commits to Clojure master on Oct 25 2013. Given that what kinds of solution methods would be acceptable for this issue, it sounds like more thinking and code changes are probably needed anyway before worrying too much about that.





[CLJ-1278] State function's unmunged full name in compiled function's toString() Created: 10/Oct/13  Updated: 17/May/15

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

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: errormsgs, interop

Attachments: Text File CLJ-1278-2.patch     Text File CLJ-1528--function-tostring.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Currently function instances print their toString() with the munged Java name:

user=> (ns proj.util-fns)
nil
proj.util-fns=> (defn a->b [a] (inc a))
#'proj.util-fns/a->b
proj.util-fns=> a->b
#object[proj.util_fns$a__GT_b 0x141ba1f1 "proj.util_fns$a__GT_b@141ba1f1"]

For debugging purposes, it would be useful to have the function toString() describe the Clojure-oriented fn name.

Approach: Store the original name in the function instance and use it in the toString() rather than returning the class name.

proj.util-fns=> a->b
#object[proj.util_fns$a__GT_b 0x47d1a507 "proj.util-fns/a->b(NO_SOURCE_FILE:2)"]

Tradeoffs: Increased function instance size for the function name.

Patch: CLJ-1278-2.patch



 Comments   
Comment by Howard Lewis Ship [ 10/Oct/13 8:39 PM ]

Contains changes and updated tests. I don't have any details on if this affects compiler performance or generated code size in any significant or even measurable way.

Comment by Andy Fingerhut [ 11/Oct/13 4:06 PM ]

Howard, sorry I do not have more useful comments on the changes you make in your patch. Right now I only have a couple of minor comments on its form. The preferred format for patches is that created using the instructions shown on this wiki page: http://dev.clojure.org/display/community/Developing+Patches

Also, there are several parts of your patch that appear to only make changes in the whitespace of lines. It would be best to leave such changes out of a proposed patch.

Comment by Howard Lewis Ship [ 11/Oct/13 5:00 PM ]

Yes, I didn't notice the whitespace changes until after; I must have hit reformat at some point, despite my best efforts. I'll put together a new patch shortly.

Comment by Howard Lewis Ship [ 11/Oct/13 6:26 PM ]

Clean patch

Comment by Howard Lewis Ship [ 25/Nov/14 6:00 PM ]

FYI, it's been a year. The correct file is CLJ-1278-2.patch.

Comment by Howard Lewis Ship [ 25/Nov/14 6:07 PM ]

... hm, something's changed in recent times.

     [java] FAIL in (fn-toString) (fn.clj:83)
     [java] nested functions
     [java] expected: (= (simple-name (.toString (factory-function))) (str "clojure.test-clojure.fn/" "factory-function/fn"))
     [java]   actual: (not (= "clojure.test-clojure.fn/factory-function/fn__7565" "clojure.test-clojure.fn/factory-function/fn"))
     [java]
     [java] FAIL in (fn-toString) (fn.clj:83)
     [java] nested functions
     [java] expected: (= (simple-name (.toString (named-factory-function))) (str "clojure.test-clojure.fn/" "named-factory-function/a-function-name"))
     [java]   actual: (not (= "clojure.test-clojure.fn/named-factory-function/a-function-name__7568" "clojure.test-clojure.fn/named-factory-function/a-function-name"))

I'd be willing to update my patch if there's any indication that it will ever be picked up. It's been over a year since last update.

Comment by Andy Fingerhut [ 26/Nov/14 10:30 AM ]

The change in behavior you are seeing is most likely due to a fix for ticket CLJ-1330.

And in case you were wondering, no, I am not the person who knows what tickets are of interest. I know that this one has gotten a fair number of votes, and by votes is one of the top ranked enhancement suggestions - look under "enhancements" on this report, or search for 1330: http://jafingerhut.github.io/clj-ticket-status/CLJ-top-tickets-by-weighted-vote.html

The features going into Clojure 1.7 are pretty well decided upon, and a fair number of other fixes and enhancements were delayed to 1.8. A longer than 1 year wait is not unusual, especially for enhancements.

Comment by Howard Lewis Ship [ 26/Nov/14 3:06 PM ]

Thanks for the info; don't want to come off as whiny but The Great Silence is off putting to someone who wants to help improve things.

I'll update my patch, and hope to see some motion for 1.8.

Comment by Alex Miller [ 26/Nov/14 3:43 PM ]

There are ~400 open tickets for Clojure. As a printing enhancement, this is generally considered lower priority than defects. Additionally, the proposal changes the compiler, bytecode generation code, and adds fields to generated objects, which has unassessed and potentially wide impacts. The combination of these things means it might be a while before we get around to looking at it.

Things that you could do to help:
1) Simplify the description. Someone coming to this ticket (screeners and ultimately Rich) want to look at the description and get the maximal understanding with the minimal effort. We have some guidelines on this at http://dev.clojure.org/display/community/Creating+Tickets if you haven't seen it. For an enhancement, a short (1-2 sentence) description of the problem and an example I can run in the repl is best. Then a proposal (again, as short as possible). Examples: CLJ-1529, CLJ-1325, CLJ-1378. For an enhancement like this, seeing (succinct) before/after versions where a user will see this is often the quickest way for a screener to understand the benefit.

2) Anticipate and remove blockers. As I mentioned above, you are changing the size of every function object. What is the impact on size and construction time? Providing data and/or a test harness saves a screener from doing this work. It's better to leave details in attachments or comments and refer to it in the description if it's lengthy.

3) Have others screen (per http://dev.clojure.org/display/community/Screening+Tickets ) - while that is the job a screener (often me) will have to re-do, having more eyeballs on it early helps. Ask on #clojure for someone else to take a look, try it, etc. If there are open questions, leaving those in the description helps guide my work.

Comment by Howard Lewis Ship [ 26/Nov/14 4:09 PM ]

Alex, thanks for the advice. I'll follow through. Some of that data is already present, but I can make it more prominent.

I know that I'm overwhelmed by the number of issues (including enhancements and minor improvements) on the Tapestry issue list, so I'm understanding of problem space.

Comment by Steve Miner [ 17/May/15 9:06 AM ]

You could instead implement toString() on something like AFn.java.

public String toString() {
    String name = getClass().getSimpleName();
    return Compiler.demunge(name);
}
Comment by Alex Miller [ 17/May/15 11:06 AM ]

Munge+demunge is a lossy operation. Consider demunge as "best effort", not something to rely on.





[CLJ-1261] Invalid defrecord results in exception attributed to namespace that imports namespace with defrecord Created: 12/Sep/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

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

Type: Defect Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord, errormsgs

Attachments: File clj-1261-2.diff     File clj-1261-3.diff     File clj-1261-4.diff     File clj-1261-5.diff    
Patch: Code and Test
Approval: Ok

 Description   

I was introducing a namespace that included a defrecord.

My defrecord was wrong; it used a keyword to define a field, not a symbol. Minimal test case:

% cat src/useclj16/init.clj
(ns useclj16.init)

(defrecord Application [:shutdown-fn])
% cat src/useclj16/app.clj 
(ns useclj16.app
  (:require [useclj16.init :as init]))

However, the exception was perplexing:

% java -cp clojure-1.6.0-master-SNAPSHOT.jar:src clojure.main
user=> (require 'useclj16.app)
ClassCastException clojure.lang.Keyword cannot be cast to clojure.lang.IObj  clojure.core/with-meta (core.clj:214)

user=> (pst *e 100)
ClassCastException clojure.lang.Keyword cannot be cast to clojure.lang.IObj
        clojure.core/with-meta (core.clj:214)
        clojure.core/defrecord/fn--147 (core_deftype.clj:362)
        clojure.core/map/fn--4210 (core.clj:2494)
        clojure.lang.LazySeq.sval (LazySeq.java:42)
        clojure.lang.LazySeq.seq (LazySeq.java:60)
        clojure.lang.RT.seq (RT.java:484)
        clojure.lang.LazilyPersistentVector.create (LazilyPersistentVector.java:31)
        clojure.core/vec (core.clj:354)
        clojure.core/defrecord (core_deftype.clj:362)
        clojure.lang.Var.invoke (Var.java:427)
        clojure.lang.Var.applyTo (Var.java:532)
        clojure.lang.Compiler.macroexpand1 (Compiler.java:6483)
        clojure.lang.Compiler.macroexpand (Compiler.java:6544)
        clojure.lang.Compiler.eval (Compiler.java:6618)
        clojure.lang.Compiler.load (Compiler.java:7079)
        clojure.lang.RT.loadResourceScript (RT.java:370)
        clojure.lang.RT.loadResourceScript (RT.java:361)
        clojure.lang.RT.load (RT.java:440)
        clojure.lang.RT.load (RT.java:411)
        clojure.core/load/fn--5024 (core.clj:5546)
        clojure.core/load (core.clj:5545)
        clojure.core/load-one (core.clj:5352)
        clojure.core/load-lib/fn--4973 (core.clj:5391)
        clojure.core/load-lib (core.clj:5390)
        clojure.core/apply (core.clj:619)
        clojure.core/load-libs (core.clj:5429)
        clojure.core/apply (core.clj:619)
        clojure.core/require (core.clj:5512)
        useclj16.app/eval322/loading--4916--auto----323 (app.clj:1)
        useclj16.app/eval322 (app.clj:1)
        clojure.lang.Compiler.eval (Compiler.java:6634)
        clojure.lang.Compiler.eval (Compiler.java:6623)
        clojure.lang.Compiler.load (Compiler.java:7079)
        clojure.lang.RT.loadResourceScript (RT.java:370)
        clojure.lang.RT.loadResourceScript (RT.java:361)
        clojure.lang.RT.load (RT.java:440)
        clojure.lang.RT.load (RT.java:411)
        clojure.core/load/fn--5024 (core.clj:5546)
        clojure.core/load (core.clj:5545)
        clojure.core/load-one (core.clj:5352)
        clojure.core/load-lib/fn--4973 (core.clj:5391)
        clojure.core/load-lib (core.clj:5390)
        clojure.core/apply (core.clj:619)
        clojure.core/load-libs (core.clj:5429)
        clojure.core/apply (core.clj:619)
        clojure.core/require (core.clj:5512)
        user/eval318 (NO_SOURCE_FILE:1)
        clojure.lang.Compiler.eval (Compiler.java:6634)
        clojure.lang.Compiler.eval (Compiler.java:6597)
        clojure.core/eval (core.clj:2864)
        clojure.main/repl/read-eval-print--6594/fn--6597 (main.clj:260)
        clojure.main/repl/read-eval-print--6594 (main.clj:260)
        clojure.main/repl/fn--6603 (main.clj:278)
        clojure.main/repl (main.clj:278)
        clojure.main/repl-opt (main.clj:344)
        clojure.main/main (main.clj:442)
        clojure.lang.Var.invoke (Var.java:411)
        clojure.lang.Var.applyTo (Var.java:532)
        clojure.main.main (main.java:37)
nil

The error was attributed to app.clj (useclj16.app), a namespace which requires useclj16.init, the namespace containing the defrecord.

No indication that this concerned a defrecord, or even what namespace contained the error, was present in the exception.

Patch: clj-1261-5.diff

Approach: Check explicitly that the fields are all symbols, for both defrecord and deftype, and throw a CompilerException with file, line, and column number if not. Example of exception after patch is applied, in the case give above:

user=> (require 'useclj16.app)
CompilerException java.lang.AssertionError: defrecord and deftype fields must be symbols, useclj16.init.Application had: :shutdown-fn, compiling:(useclj16/init.clj:3:1)

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 12/Sep/13 8:58 PM ]

Can you include an example of the defrecord definition just so we're clear what it looks like?

Comment by Alex Miller [ 12/Sep/13 8:59 PM ]

Also, "feedback" is not a useful label. Please use "errormsgs" for stuff like this. See the list of many commonly used labels here: http://dev.clojure.org/display/community/Creating+Tickets

Comment by Howard Lewis Ship [ 13/Sep/13 10:42 AM ]

"Feedback" is my own personal crusade http://tapestryjava.blogspot.com/2013/05/once-more-feedback-please.html

In my case, my invalid code was:

(defrecord Application [:shutdown-fn])

And the mistake was that :shutdown-fn should be a symbol, not a keyword.

Here it is, more completely:

(ns novate.services.initialization
  "Infrastructure for system-as-transient state.")

(defrecord Application [:shutdown-fn])

and

(ns novate.services.activator
  "Responsible for bootstrapping the application by loading certain namespaces and invoking certain functions, guided by data in JAR manifests."
  (:gen-class)
  (:require [clojure.edn :as edn]
            [clojure.java.io :as io]
            [novate.util.logging :as l]
            [novate.services
             [initialization :as init]
             [ordering :as o]])
  (:import [java.io PushbackReader]))

The error was attributed to this file.

Comment by Andy Fingerhut [ 13/Sep/13 11:48 AM ]

Patch clj-1261-v1.txt throws an exception if any fields given to defrecord or deftype are not symbols. They are CompilerExceptions, so include an accurate file, line, and column number.

Comment by Andy Fingerhut [ 13/Sep/13 11:57 AM ]

Updated description to give minimal test case.

Comment by Andy Fingerhut [ 23/Nov/13 1:06 AM ]

Patch clj-1261-2.diff is identical to clj-1261-v1.txt except that it applies cleanly to latest master. The only change was to some lines of context due to recent commits to Clojure.

Comment by Alex Miller [ 18/Apr/14 1:16 PM ]

I think the patch is ok but I have two suggestions in the error message - first, include the record/type ns+name (I think the classname in the patched fn is what you want). Second, I think the wording could be adjusted a bit and the parens should go away - those look like but don't actually have meaning in the original context (since you are filtering out the symbols). Maybe something like:

"defrecord and deftype fields must be symbols, useclj16.init.Application had: :shutdown-fn, :foo-bar"

Comment by Andy Fingerhut [ 30/Apr/14 9:32 AM ]

Patch clj-1261-3.diff attempts to incorporate Alex's suggested error message changes.

There are other errors caught by function validate-fields that could have more details like the namespace and record/type name added to them, but I don't want to go out of scope for the ticket. I can create another patch that does that if there is interest.

Comment by Alex Miller [ 30/Apr/14 2:56 PM ]

Can you update the "after" example in the Approach section of the description to match new?

Comment by Andy Fingerhut [ 01/May/14 4:18 PM ]

Updated example output at end of description to be what is seen after patch clj-1261-3.diff is applied.

Comment by Alex Miller [ 05/May/14 1:56 PM ]

Description looks good, patch looks good. Test?

Comment by Andy Fingerhut [ 05/May/14 2:03 PM ]

I'd be happy to write one, if I had a "similar" one to pattern them on.

By similar, I mean: are there any existing tests that require a namespace that isn't already loaded & compiled when the tests begin running, and catch exceptions thrown during the require?

Comment by Alex Miller [ 05/May/14 3:56 PM ]

I think you should be able to test the right error message here by just invoking the defrecord form.

Otherwise, maybe https://github.com/clojure/clojure/blob/master/test/clojure/test_clojure/ns_libs.clj#L87 ?

Comment by Andy Fingerhut [ 10/May/14 6:43 PM ]

Patch clj-1261-4.diff is identical to clj-1261-3.diff except that it adds a couple of unit tests verifying that an exception of the desired type and with an appropriate message is thrown when keywords are used as defrecord or deftype fields.

Comment by Alex Miller [ 13/May/14 10:41 PM ]

same as -4 but changed final defrecord to deftype in test (seemed like a typo)

Comment by Andy Fingerhut [ 13/May/14 11:40 PM ]

Thanks for the catch on that typo in the tests. You changed it to what I had intended.

Comment by Alex Miller [ 13/May/14 11:54 PM ]

seemed pretty clear





[CLJ-1249] Warning when a record field with the same name as a function exists for it Created: 26/Aug/13  Updated: 30/Dec/14  Resolved: 31/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Benjamin Peter Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: errormsgs
Environment:

Linux, clojure jar or leiningen repl



 Description   

Hi,

I had the following problem which took me much longer than it should have. I accidentally had a record's field with the same name as one of the functions from a protocol. When I tried to call the function from within the record I got totally weird behavior and didn't find it, until I removed every piece of code when I found the name conflict.

I wish clojure would warn me in such a case. (Disregarding any naming conventions that could have saved me.)

Following a small example to reproduce the problem:

(defprotocol HasPets
  (dogs [this])
  (cats [this])
  (octopus [this])
  (cute-ones [this]))

; Here the field "dog" is added with the same name as the protocol
(defrecord Petshop [dogs] 
  HasPets
  (dogs [this]
    [:pluto :bethoven])
  (cats [this]
    [:tom])
  (octopus [this]
    [:henry])
  (cute-ones [this]
    ; Here it was intended to call the function "dogs", instead the
    ; field is used.
    (concat (dogs this) (cats this))))

(def dogs-in-stock nil)

(let [petshop (->Petshop dogs-in-stock)]
  (println "Dogs for sale:" (dogs petshop))
  (println "All cute animals: " (cute-ones petshop)))

Results in:

clojure core.clj
Dogs for sale: [:pluto :bethoven]
Exception in thread "main" java.lang.NullPointerException
        at user.Petshop.cute_ones(core.clj:20)
        at user$eval84.invoke(core.clj:26)
        at clojure.lang.Compiler.eval(Compiler.java:6514)
        at clojure.lang.Compiler.load(Compiler.java:6955)
        at clojure.lang.Compiler.loadFile(Compiler.java:6915)
        at clojure.main$load_script.invoke(main.clj:283)
        at clojure.main$script_opt.invoke(main.clj:343)
        at clojure.main$main.doInvoke(main.clj:427)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at clojure.lang.Var.invoke(Var.java:415)
        at clojure.lang.AFn.applyToHelper(AFn.java:161)
        at clojure.lang.Var.applyTo(Var.java:532)
        at clojure.main.main(main.java:37)

Expected warning:

Warning: the protocol function "dog" from "HasPets" conflicts with the equally named record field of "Petshop"

Without dog as a record's field:

clojure core.clj
Dogs for sale: [:pluto :bethoven]
All cute animals:  (:pluto :bethoven :tom)

Thanks for your help.

Ben.



 Comments   
Comment by Alex Miller [ 26/Aug/13 1:20 PM ]

Thanks for the report.

It seems like there is a scoping issue here where dogs is bound to the field and there is also a context for it being referred to as a function. It's possible that this should really be treated as a compiler bug and not a warning message problem - that requires some more detective work. Since there is only one function dogs (the field name is a function only in keyword form - :dogs), it seems unambiguous what should be done here.

In the meanwhile, presumably a workaround would be to use extend-protocol, etc to attach the protocol to the record outside the record definition.

Comment by Gary Fredericks [ 09/Dec/13 8:24 AM ]

Alex I can't see a lack of ambiguity here. I read your comment as saying that because dogs is in the call position we can deduce that it's meant to refer to the function rather than the field. But we can't assume that a field is not an IFn or intended to be used as one, so I can't make sense of that.

Another workaround should be using a fully qualified reference to the protocol function.

My expectation as a user of defrecord and deftype has always been that ambiguous references always refer to the fields, as if there were a let around each function body. So I've done this kind of thing intentionally I think. I don't know what that means about whether or not it should be a warning.

Should warnings for this kind of thing be accompanied by a way to suppress individual instances of the warning? E.g., a ^:no-warn metadata somewhere?

Comment by Stuart Halloway [ 31/Jan/14 2:41 PM ]

It is correct and legal code to have a record or type field shadow a var name, and as Gary mentions, the var is still reachable via a qualified name.

This might be a good warning for a linter like https://github.com/jonase/eastwood, if it is not present already.

Comment by Andy Fingerhut [ 31/Jan/14 2:55 PM ]

The Eastwood linter currently has no warning for this situation, but I have created an issue on its Github page to record the enhancement idea: https://github.com/jonase/eastwood/issues/55

Comment by Benjamin Peter [ 31/Jan/14 3:26 PM ]

Okay, thanks guys.

Comment by Andy Fingerhut [ 25/Dec/14 12:15 PM ]

I started looking at implementing this in Eastwood today, only to discover that the :local-shadows-var linter implemented in an earlier release on Nov 4 2014 already catches this case and warns about it, among other things. The latest release also works fine for catching this, too.

Comment by Benjamin Peter [ 30/Dec/14 3:10 PM ]

Thanks!

Tried it now and got a warning - didn't use eastwood before and tbh would expect it to be a part of the default clojure runtime.

Comment by Benjamin Peter [ 30/Dec/14 3:11 PM ]

Output for the example:

src/petshop/core.clj:21:13: local-shadows-var: local: dogs invoked as function shadows var: #'petshop.core/dogs




[CLJ-1248] Show type information in reflection warning messages when available Created: 24/Aug/13  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Christoffer Sawicki Assignee: Unassigned
Resolution: Completed Votes: 12
Labels: errormsgs

Attachments: Text File clj-1248-2.patch     Text File Include-type-information-in-reflection-warning-messa.patch    
Patch: Code and Test
Approval: Ok

 Description   

The reflection warning messages currently don't show any type information. I think adding this would make the messages more helpful by making it more obvious what the problem is. I suggest these changes:

(set! *warn-on-reflection* true)

(defn foo [^String x] (.blah x))
Before: reference to field blah can't be resolved.
After:  reference to field blah on java.lang.String can't be resolved.

(defn foo [^String x] (.zap x 1))
Before: call to zap can't be resolved.
After:  call to method zap on java.lang.String can't be resolved (no such method).

(defn foo [] (Integer/valueOf #"boom"))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf on java.lang.Integer can't be resolved (argument types: java.util.regex.Pattern).

(defn foo [x] (Integer/valueOf x))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf on java.lang.Integer can't be resolved (argument types: unknown).

Patch: clj-1248-2.patch



 Comments   
Comment by Alex Miller [ 25/Aug/13 8:31 AM ]

I think these are a good idea. I think it would be better to separate the reflected class from the field though since we are referring to fields that don't exist.

For example:
1) field: "reference to field blah in java.lang.String can't be resolved."
2) method: "call to method zap in java.lang.String can't be resolved."
3) static method: "call to method valueOf in java.lang.Integer can't be resolved."

Your 3rd example actually highlights something more interesting though. In this case the problem is not actually that Integer/valueOf does not exist but rather that it is being called with the wrong types. In these cases, what I want to know is: what is the type of the parameters being passed.

In #2 there are actually two possible sources of error - an unexpected type for x or an unexpected type or arity for the parameters. It would be useful to check whether the method of this name exists and at what arity to determine which of these cases exists and give a more precise error.

In any case, the implementation needs to be supplied as a patch, not a link. See: http://dev.clojure.org/display/community/Developing+Patches

Comment by Christoffer Sawicki [ 25/Aug/13 3:25 PM ]

+1 on all points. I'll begin work on an updated patch.

Yes, there is a deeper more interesting problem lurking here.

One question is what should result in reflection warnings and what in compile-time errors. (I think some types of reflection warnings should be promoted to errors.)

Here are some examples of current behavior with comments:

(fn [] (Integer/valueOff :foo))
CompilerException java.lang.IllegalArgumentException: No matching method: valueOff, compiling:(NO_SOURCE_PATH:1:8) 

^ Error because the compiler can statically see this is never going to work. Fine.

(fn [] (Integer/valueOf :foo))
Reflection warning, NO_SOURCE_PATH:1:8 - call to valueOf can't be resolved.

^ This is never going to work either, but only gives a warning. A bit surprising; I'd prefer an error.

(fn [x] (.foo x))
Reflection warning, NO_SOURCE_PATH:1:9 - reference to field foo can't be resolved.

^ This could work. Fine.

(fn [^String x] (.foo x))
Reflection warning, NO_SOURCE_PATH:1:17 - reference to field foo can't be resolved.

^ This is never going to work if x is a String but x can be of any type at run-time. Personally, I think this should be an error...

Comment by Alex Miller [ 25/Aug/13 4:36 PM ]

You should take any warning/error differences to one (or more) new tickets where they can be evaluated individually. The more you put in one ticket, the more likely it is to get bogged down and/or rejected. My gut feeling is that there would not be a lot of support for the warning->error changes you suggest.

Comment by Christoffer Sawicki [ 28/Aug/13 3:05 PM ]

Here's an updated patch that changes the messages like this:

(defn foo [^String x] (.blah x))
Before: reference to field blah can't be resolved.
After:  reference to field blah of java.lang.String can't be resolved.

(defn foo [^String x] (.zap x 1))
Before: call to zap can't be resolved.
After:  call to method zap of java.lang.String can't be resolved (no such method).

(defn foo [] (Integer/valueOf #"boom"))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf of java.lang.Integer can't be resolved (argument types: [java.util.regex.Pattern]).

(defn foo [x] (Integer/valueOf x))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf of java.lang.Integer can't be resolved (argument types: [unknown]).

(I wish I could edit the issue description.)

Comment by Alex Miller [ 28/Aug/13 3:18 PM ]

Updated description per latest patch.

Comment by Alex Miller [ 12/Feb/14 1:12 AM ]

I used this in a local build to resolve an issue tonight and found it very helpful. One comment I have is that in the message part "(argument types: [java.util.regex.Pattern])", I would like to remove the outer [ ] around the argument types. In the case I was working on the first type was actually a long[] which has class name "[J" so I found the outer []s distracting.

Comment by Christoffer Sawicki [ 12/Feb/14 5:41 AM ]

Thanks for the success story!

I think I choose to use a vector to disambiguate the case with one argument that is unknown, i.e. "(argument types: [unknown])". Without the vector, it's not obvious if there's one argument of unknown type or if all of multiple arguments are of unknown type.

Given your input and some more thought, it's probably not worth making every other message worse for this single case. I'll update the patch tonight.

Comment by Alex Miller [ 12/Feb/14 9:49 AM ]

I went ahead and updated the patch. I also fixed a couple whitespace issues and changed the word "of" to "on" before the type as I think it reads better.

Comment by Christoffer Sawicki [ 12/Feb/14 1:57 PM ]

OK, thanks!





[CLJ-1210] error message for (clojure.java.io/reader nil) — consistency for use with io/resource Created: 23/May/13  Updated: 11/May/15

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

Type: Enhancement Priority: Major
Reporter: Trevor Wennblom Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: checkargs, errormsgs, ft, io

Attachments: File extend-io-factory-to-nil.diff    
Patch: Code and Test
Approval: Triaged

 Description   

This seems to be a common idiom:

(clojure.java.io/reader (clojure.java.io/resource "myfile"))

When a file is available these are the behaviors:

=> (clojure.java.io/reader "resources/myfile")
#<BufferedReader java.io.BufferedReader@1f291df0>

=> (clojure.java.io/resource "myfile")
#<URL file:/project/resources/myfile>

=> (clojure.java.io/reader (clojure.java.io/resource "myfile"))
#<BufferedReader java.io.BufferedReader@1db04f7c>

If the file (resource) is unavailable:

=> (clojure.java.io/reader "resources/nofile")
FileNotFoundException resources/nofile (No such file or directory)  java.io.FileInputStream.open (FileInputStream.java:-2)

=> (clojure.java.io/resource "nofile")
nil

=> (clojure.java.io/reader (clojure.java.io/resource "nofile"))
IllegalArgumentException No implementation of method: :make-reader of protocol: #'clojure.java.io/IOFactory found for class: nil  clojure.core/-cache-protocol-fn (core_deftype.clj:541)
;; EXPECTED: better error type/message

The main enhancement request is to have a better error message from `(clojure.java.io/reader nil)`.

Approach: Extend IOFactory to nil, providing error messages consistent with the default error messages provided for Object. After:

user=> (clojure.java.io/reader (clojure.java.io/resource "nofile"))
IllegalArgumentException Cannot open <nil> as a Reader.  clojure.java.io/fn--9213 (io.clj:290)

Patch: extend-io-factory-to-nil.diff



 Comments   
Comment by Alexander Redington [ 14/Feb/14 3:13 PM ]

This patch extends IOFactory to nil, providing error messages consistent with the default error messages provided for Object.

Comment by Benjamin Peter [ 15/Feb/14 1:31 PM ]

Looks like a good solution to me as a user. Thanks for the effort!

Comment by Dennis Schridde [ 12/Jul/14 2:01 AM ]

I would also be interested in a solution, as I am currently running into this with the ClojureScript compiler.





[CLJ-1207] Importing a class that does not exist fails to report the name of the class that did not exist Created: 29/Apr/13  Updated: 02/Aug/15  Resolved: 02/Aug/15

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

Type: Defect Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Not Reproducible Votes: 1
Labels: errormsgs
Environment:

1.5.1, OS X


Waiting On: Howard Lewis Ship

 Description   

Pop quiz: What Java class is missing from the classpath?

java.lang.NoClassDefFoundError: Could not initialize class com.annadaletech.nexus.util.logging__init
 at java.lang.Class.forName0 (Class.java:-2)
    java.lang.Class.forName (Class.java:264)
    clojure.lang.RT.loadClassForName (RT.java:2098)
    clojure.lang.RT.load (RT.java:430)
    clojure.lang.RT.load (RT.java:411)
    clojure.core$load$fn__5018.invoke (core.clj:5530)
    clojure.core$load.doInvoke (core.clj:5529)
    clojure.lang.RestFn.invoke (RestFn.java:408)
    clojure.core$load_one.invoke (core.clj:5336)
    clojure.core$load_lib$fn__4967.invoke (core.clj:5375)
    clojure.core$load_lib.doInvoke (core.clj:5374)
    clojure.lang.RestFn.applyTo (RestFn.java:142)
    clojure.core$apply.invoke (core.clj:619)
    clojure.core$load_libs.doInvoke (core.clj:5413)
    clojure.lang.RestFn.applyTo (RestFn.java:137)
    clojure.core$apply.invoke (core.clj:619)
    clojure.core$require.doInvoke (core.clj:5496)
    clojure.lang.RestFn.invoke (RestFn.java:512)
    novate.console.app$eval1736$loading__4910__auto____1737.invoke (app.clj:1)
    novate.console.app$eval1736.invoke (app.clj:1)
    clojure.lang.Compiler.eval (Compiler.java:6619)
    clojure.lang.Compiler.eval (Compiler.java:6608)
    clojure.lang.Compiler.load (Compiler.java:7064)
    user$eval1732.invoke (NO_SOURCE_FILE:1)
    clojure.lang.Compiler.eval (Compiler.java:6619)
    clojure.lang.Compiler.eval (Compiler.java:6582)
    clojure.core$eval.invoke (core.clj:2852)
    clojure.main$repl$read_eval_print__6588$fn__6591.invoke (main.clj:259)
    clojure.main$repl$read_eval_print__6588.invoke (main.clj:259)
    clojure.main$repl$fn__6597.invoke (main.clj:277)
    clojure.main$repl.doInvoke (main.clj:277)
    clojure.lang.RestFn.invoke (RestFn.java:1096)
    clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__584.invoke (interruptible_eval.clj:56)
    clojure.lang.AFn.applyToHelper (AFn.java:159)
    clojure.lang.AFn.applyTo (AFn.java:151)
    clojure.core$apply.invoke (core.clj:617)
    clojure.core$with_bindings_STAR_.doInvoke (core.clj:1788)
    clojure.lang.RestFn.invoke (RestFn.java:425)
    clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invoke (interruptible_eval.clj:41)
    clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn__625$fn__628.invoke (interruptible_eval.clj:171)
    clojure.core$comp$fn__4154.invoke (core.clj:2330)
    clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__618.invoke (interruptible_eval.clj:138)
    clojure.lang.AFn.run (AFn.java:24)
    java.util.concurrent.ThreadPoolExecutor.runWorker (ThreadPoolExecutor.java:1110)
    java.util.concurrent.ThreadPoolExecutor$Worker.run (ThreadPoolExecutor.java:603)
    java.lang.Thread.run (Thread.java:722)

If you guess "com.annadaletech.nexus.util.logging__init" you are wrong!

Wait, I'll give you a hint:

(ns com.annadaletech.nexus.util.logging
  (:use [clojure.string :only [trim-newline]]
        [clojure.pprint :only [code-dispatch pprint with-pprint-dispatch *print-right-margin*]])
  (:import [java.io StringWriter]
           [org.slf4j MDC MarkerFactory Marker LoggerFactory]
           [java.util.concurrent.locks ReentrantLock]))

Oh, sorry, did that not help?

The correct answer is "org.slf4j.MDC".

Having that information in the stack trace would have saved me nearly an hour. I think it is worth the effort to get that reported correctly.



 Comments   
Comment by Gabriel Horner [ 10/May/13 1:56 PM ]

When I try this on a fresh project, I get this error:
"ClassNotFoundException org.slf4j.MDC
java.net.URLClassLoader$1.run (URLClassLoader.java:202)
java.security.AccessController.doPrivileged (AccessController.java:-2)"

Howard, could you give us a project.clj or better yet a github repository that recreates this issue?

Comment by Howard Lewis Ship [ 10/May/13 4:51 PM ]

I'll see what I can do. Probably be next week. Thanks for looking at this.

Comment by Gary Fredericks [ 26/May/13 8:20 AM ]

This reminds me of an issue with `lein run` that resulted from it trying to figure out whether you wanted to run a namespace or a java class:

https://github.com/technomancy/leiningen/issues/1182

Comment by Alex Miller [ 02/Aug/15 8:23 PM ]

I tried a few things but there is not enough info here for me to reproduce it. Please re-open if you can do so.





[CLJ-1189] Map-destructuring :or fumble needs compiler warning Created: 31/Mar/13  Updated: 02/Aug/15

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs

Attachments: Text File CLJ-1189-p1.patch    
Patch: Code and Test

 Description   

Here is a map-destructuring blunder that I wish the compiler warned about:

(defn server
[{servlet ::servlet
type ::type
:or {::type :jetty}
:as service-map}]

It would be splendid to get a warning that :or keys that are not symbols being bound have no effect.

The incomplete code snippet above comes from Pedestal.service 0.1.0.

Here is a complete one-line example with the coding error:

user> (defn picnic [{botulism :botulism :or {:botulism 6}}] botulism)
#'user/picnic
user> (picnic {})
nil
user> ;; I intended 6.



 Comments   
Comment by Gary Fredericks [ 26/May/13 8:25 AM ]

Should this be a warning or an exception? I don't know of any similar example of the compiler giving a warning, so I would expect the latter.

Comment by Gary Fredericks [ 26/May/13 9:54 AM ]

Added a patch that throws an exception when :or is not a map or its keys are not symbols. Also some tests.





[CLJ-1169] Report line,column, and source in defmacro errors Created: 22/Feb/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

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

Type: Enhancement Priority: Major
Reporter: Andrei Kleschinski Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: errormsgs
Environment:

Windows


Attachments: Text File 0001-CLJ-1169-proposed-patch.patch     Text File 0002-CLJ-1169-fix-unit-tests.patch     Text File CLJ-1169-code-and-test-1.patch     File defn_error_message.clj    
Patch: Code
Approval: Ok

 Description   

Summary This patch grew out of a desire to have defn report filename and line numbers for parameter declaration errors, but the approach chosen does something more broad, and likely very useful: Anytime defmacro is throwing a non-CompilerException, wrap it in a CompilerException that captures LINE, COLUMN, and SOURCE. Presumably this would improve reporting for many other macros as well. The patch also tweaks errors messages to add quotes, e.g. "problem" instead of problem, which seems useful.

Screened By Stu
Patch CLJ-1169-code-and-test-1.patch, which aggregates the work in other patches to a single patch that works on current master.

When mistyping parameter list in defn declaration, e.g.

(defn test
 (some-error))

error message shows name of parameter (without quotes), but not function name, filename or line number:

Exception in thread "main" java.lang.IllegalArgumentException: Parameter declaration some-error should be a vector
        at clojure.core$assert_valid_fdecl.invoke(core.clj:6567)
        at clojure.core$sigs.invoke(core.clj:220)
        at clojure.core$defn.doInvoke(core.clj:294)
        at clojure.lang.RestFn.invoke(RestFn.java:467)
        at clojure.lang.Var.invoke(Var.java:427)
        at clojure.lang.AFn.applyToHelper(AFn.java:172)
        at clojure.lang.Var.applyTo(Var.java:532)
        at clojure.lang.Compiler.macroexpand1(Compiler.java:6366)
        at clojure.lang.Compiler.macroexpand(Compiler.java:6427)
        at clojure.lang.Compiler.eval(Compiler.java:6495)
        at clojure.lang.Compiler.load(Compiler.java:6952)
        at clojure.lang.Compiler.loadFile(Compiler.java:6912)
        at clojure.main$load_script.invoke(main.clj:283)
        at clojure.main$init_opt.invoke(main.clj:288)
        at clojure.main$initialize.invoke(main.clj:316)
        at clojure.main$null_opt.invoke(main.clj:349)
        at clojure.main$main.doInvoke(main.clj:427)
        at clojure.lang.RestFn.invoke(RestFn.java:421)
        at clojure.lang.Var.invoke(Var.java:419)
        at clojure.lang.AFn.applyToHelper(AFn.java:163)
        at clojure.lang.Var.applyTo(Var.java:532)
        at clojure.main.main(main.java:37)


 Comments   
Comment by Andrei Kleschinski [ 22/Feb/13 5:39 AM ]

Proposed patch for issue
Process exceptions in macroexpand1 and wraps them in CompilerException with source,line,column information.

Also patch adds quotes around invalid symbol name in error message to make them more distinguishable from rest of message.

Comment by Andy Fingerhut [ 01/Mar/13 9:32 AM ]

Patch 0001-CLJ-1169-proposed-patch.patch dated Feb 22 2013 causes several tests to fail. Run "./antsetup.sh" then "ant" to see which ones (or "mvn package").

Comment by Andrei Kleschinski [ 01/Mar/13 10:25 AM ]

Fix for failed unit-tests

Comment by Stuart Halloway [ 27/Jun/14 2:40 PM ]

Andrei, can you please sign the CA (e-form at http://clojure.org/contributing) so that we can consider this patch?

Thanks!

Comment by Andrei Kleschinski [ 27/Jun/14 3:05 PM ]

Ok, I have signed the CA.

Comment by Alex Miller [ 27/Jun/14 4:06 PM ]

I can confirm that Andrei has signed the CA. Back in Vetted.





[CLJ-1162] Error Message when calling deref on a non-IDeref is unhelpful Created: 12/Feb/13  Updated: 03/Sep/13

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

Type: Enhancement Priority: Minor
Reporter: Julian Birch Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs
Environment:

Found this on ubuntu, lein 2, Clojure 1.5 snapshot, but it's obvious from inspection


Attachments: Text File CLJ-1162-p1.patch    
Patch: Code

 Description   

If you just type "@1" is the repl, on previous versions you'll get an error that Long cannot be cast to IDeref. In 1.5, the error message is that it cannot be cast to java.util.concurrent.Future. This is because it assumes that anything that isn't an IDeref is automatically a Future. The deref method should generate a custom error stating that the type you've passed in is neither an IDeref nor a Future.



 Comments   
Comment by Gary Fredericks [ 26/May/13 3:00 PM ]

Attached a patch that implements the old behavior (can't cast to IDeref), which strikes me as good enough considering the support for j.u.c.Future seems rather an edge case (being that clojure's futures are themselves IDeref).

The weirdest thing I did was to use clojure.core/cast to unconditionally throw a ClassCastException. Let me know if that's weird and I'll do something different.





[CLJ-1159] clojure.java.io/delete-file doesn't return the status of the deletion(true/false) Created: 10/Feb/13  Updated: 21/Jul/15

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

Type: Defect Priority: Minor
Reporter: AtKaaZ Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, io
Environment:

any


Approval: Triaged

 Description   

initially reported it here(somehow):
https://groups.google.com/d/msg/clojure/T9Kvr0IL0kg/wcKBfR9w_1sJ

Basically clojure.java.io/delete-file doesn't ever return false (even when silently is true, it returns the value of silently), it's due to how it's implemented - but it's obvious from the code, so I'll stop here.

Thanks.

PS: this is what I'm using as my current workaround:
(defn delete-file
"
an implementation that returns the true/false status
which clojure.java.io/delete-file doesn't do(tested in 1.5.0-RC14)
"
[f & [silently]]
(let [ret (.delete (clojure.java.io/file f))]
(cond (or ret silently)
ret
:else
(throw (java.io.IOException. (str "Couldn't delete " f)))
)
)
)

I'm sure you guys can find a better way, but as a clojure newbie(really!) that's what I have.



 Comments   
Comment by AtKaaZ [ 10/Feb/13 2:01 PM ]

I kinda just realized it affects all versions since and including 1.2, because it appears that its implementation was the same since then.

If it's not meant to return the result of the delete, maybe it should specifically return nil and/or the doc say something?

Comment by Sean Corfield [ 10/Feb/13 2:21 PM ]

As noted in a thread on the Clojure ML, you can pass a known value in the second argument position to detect a delete that failed:

(clojure.java.io/delete-file some-file :not-deleted)

This returns true on success and :not-deleted on failure.

However the docstring could be better worded to make that intention clear. Perhaps:

Delete file f. Return true if it succeeds. If silently is nil or false, raise an exception if it fails, else return the value of silently.
This allows you to detect whether the delete succeeded without catching an exception by passing a non-true truthy value as the second argument.

Comment by Stuart Halloway [ 21/Jul/15 8:20 AM ]

I think 'If silently is nil or false, raise an exception if it fails, else return the value of silently.' is sufficient





[CLJ-1149] Unhelpful error message from :use (and use function) when arguments are malformed Created: 17/Jan/13  Updated: 28/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: errormsgs

Approval: Triaged

 Description   

the following exception happens when you have something like this(bad):

(ns runtime.util-test
(:use [midje.sweet :reload-all]))

as opposed to any of these(correct):

(ns runtime.util-test
(:use midje.sweet :reload-all))

(ns runtime.util-test
(:use [midje.sweet] :reload-all))

and the exception is:
Exception in thread "main" java.lang.IllegalArgumentException: No value supplied for key: true
at clojure.lang.PersistentHashMap.create(PersistentHashMap.java:77)
at clojure.core$hash_map.doInvoke(core.clj:365)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:617)
at clojure.core$load_lib.doInvoke(core.clj:5352)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:619)
at clojure.core$load_libs.doInvoke(core.clj:5403)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:621)
at clojure.core$use.doInvoke(core.clj:5497)

Note that this is similar to the equally unhelpful message shown in http://dev.clojure.org/jira/browse/CLJ-1140 although that is a different root cause.

Probably best to enhance the `use` function to validate its arguments before trying to apply hash-map?



 Comments   
Comment by Gary Fredericks [ 26/May/13 3:17 PM ]

I believe this applies to require as well.





[CLJ-1146] Symbol name starting with digits to defn throws "Unmatched delimiter )" Created: 13/Jan/13  Updated: 18/Apr/14

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

Type: Enhancement Priority: Trivial
Reporter: Linus Ericsson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, reader
Environment:

$java -jar clojure-1.5.0-RC2.jar

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



 Description   

When trying to use an invalid symbol name when defining a function, the error message thrown is a confusing and wrong one. The error message is "RuntimeException Unmatched delimiter: ) clojure.lang.Util.runtimeException (Util.java:219)", which unfortunately is the only message seen in nrepled emacs.

$ java -jar clojure-1.5.0-RC2.jar
Clojure 1.5.0-RC2
user=> (defn 45fn [] nil)
NumberFormatException Invalid number: 45fn clojure.lang.LispReader.readNumber (LispReader.java:255)
[]
nil
RuntimeException Unmatched delimiter: ) clojure.lang.Util.runtimeException (Util.java:219)

Expected:
When trying to (defn or (def a thing with a non valid symbol name, the last thrown error message should be one stating that the given symbol name is not a valid one.



 Comments   
Comment by Kevin Downey [ 18/Apr/14 2:27 AM ]

this is an artifact of how streams and repls work.

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

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

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





[CLJ-1131] Importing a non-existent class causes an exception that does not fully identify the source file Created: 17/Dec/12  Updated: 21/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs


 Description   

I'm in the process of stripping out some OSGi support, and I missed one import.

The exception identifies "init.clj", but I'd prefer to see the full path there, as I have a few different "init.clj" files in my overall project.

:core-services:compileClojure
Reflection warning, com/annadaletech/nexus/services/registry.clj:37 - call to unregisterAll can't be resolved.
Reflection warning, com/annadaletech/nexus/services/registry.clj:131 - call to getConfiguration can't be resolved.
Reflection warning, com/annadaletech/nexus/services/registry.clj:150 - call to getConfiguration can't be resolved.
Exception in thread "main" java.lang.ClassNotFoundException: org.osgi.framework.ServiceRegistration, compiling:(init.clj:1)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3387)
	at clojure.lang.Compiler.compile1(Compiler.java:7035)
	at clojure.lang.Compiler.compile1(Compiler.java:7025)
	at clojure.lang.Compiler.compile(Compiler.java:7097)
	at clojure.lang.RT.compile(RT.java:387)
	at clojure.lang.RT.load(RT.java:427)
	at clojure.lang.RT.load(RT.java:400)
	at clojure.core$load$fn__4890.invoke(core.clj:5415)
	at clojure.core$load.doInvoke(core.clj:5414)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5227)
	at clojure.core$compile$fn__4895.invoke(core.clj:5426)
	at clojure.core$compile.invoke(core.clj:5425)
	at clojuresque.tasks.compile$main$fn__64.invoke(compile.clj:23)
	at clojuresque.cli$with_command_line_STAR_.invoke(cli.clj:92)
	at clojuresque.tasks.compile$main.doInvoke(compile.clj:6)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:601)
	at clojure.lang.Var.invoke(Var.java:419)
	at clojuresque.Driver.main(Driver.java:39)
Caused by: java.lang.ClassNotFoundException: org.osgi.framework.ServiceRegistration


 Comments   
Comment by Gabriel Horner [ 17/May/13 3:56 PM ]

While it's reasonable to want this for your case, having long path names in a stacktrace could be inconvenient for others. I'd recommend posting your desired change on the dev list - https://groups.google.com/forum/?fromgroups#!forum/clojure-dev . If they're ok with it, then I'd recommend submitting a patch.

Comment by Stuart Halloway [ 21/Jul/15 1:17 PM ]

Enhanced error reporting is always an enhancement request, not a defect.





[CLJ-1130] When unable to match a static method, report arity caller was looking for, avoid misleading field error Created: 17/Dec/12  Updated: 21/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, ft

Attachments: Text File clj-1130-v1.txt     File clj-1130-v2.diff     File clj-1130-v2-ignore-ws.diff     Text File clj-1130-v2.txt     File clj-1130-v3.diff     File clj-1130-v4.diff     File clj-1130-v5.diff    
Patch: Code and Test
Approval: Vetted

 Description   

Incorrectly invoking a static method with 0 parameters yields a NoSuchFieldException:

user=> (Long/parseLong)
CompilerException java.lang.NoSuchFieldException: parseLong, compiling:(NO_SOURCE_PATH:1:1) 
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: No matching method: parseLong, compiling:(NO_SOURCE_PATH:2:1)

Approach: Error reporting enhanced to report desired arg count and to avoid a field exception confusion if 0 arg method.

user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: No matching method parseLong found taking 0 args for class java.lang.Long, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: No matching method parseLong found taking 3 args for class java.lang.Long, compiling:(NO_SOURCE_PATH:2:1)

Patch: clj-1130-v5.diff

Screened by: Alex Miller



 Comments   
Comment by Michael Drogalis [ 06/Jan/13 6:44 PM ]

It looks like it's first trying to resolve a field by name, since field access with / is legal. For example:

user=> (Integer/parseInt)
CompilerException java.lang.NoSuchFieldException: parseInt, compiling:(NO_SOURCE_PATH:1)

user=> (Integer/MAX_VALUE)
2147483647

Would trying to resolve a method before a field fix this?

Comment by Alex Miller [ 03/Sep/13 10:10 AM ]

Similarities to CLJ-1248 (there a warning, here an error).

Comment by Andy Fingerhut [ 09/Sep/13 12:36 AM ]

Patch clj-1130-v1.txt changes the error message in a situation when one attempts to invoke a static method with no args, and there is no such 0-arg static method. The message now says that there is no such method with that name and 0 args, rather than that there is no such static field with that name.

Comment by Alex Miller [ 04/Oct/13 3:56 PM ]

I updated the patch to simplify it a bit but more importantly to remove the check by exception and instead use the Reflector method that can answer this question.

Comment by Andy Fingerhut [ 12/Oct/13 3:11 PM ]

Alex, thank you for the improvements to the code. It looks better to me.

Comment by Rich Hickey [ 25/Oct/13 7:30 AM ]

due to indentation changes, this patch appears to touch much more than it probably does, making it difficult to approve.

Comment by Andy Fingerhut [ 25/Oct/13 10:59 AM ]

Any suggestions on what can be done to make progress here? Would it help to attach a patch made with "-w" option to ignore lines that differ only in whitespace? Provide git diff command line options that do this, after the patch is applied to your local workspace? Make a patch that leaves the indentation 'incorrect' after the change (involuntary shudder)?

Comment by Alex Miller [ 25/Oct/13 11:17 AM ]

The indentation has intentionally changed because the if/else structure has changed. I don't think making the patch incorrect to reduce changes is a good idea.

Comment by Andy Fingerhut [ 25/Oct/13 11:32 AM ]

Well, the 'incorrect' was in quotes because I was asking about a proposed patch that had the correct logic, but misleading indentation. Agreed it isn't a good idea, hence the shudder. I'm just brainstorming ideas to make the patch less difficult to approve.

Comment by Howard Lewis Ship [ 25/Oct/13 11:43 AM ]

At some point, you may need to bite the bullet and reformat some of the Clojure code .... Compiler.java had a crazy mix of tabs, spaces, and just completely wrong stuff.

Comment by Alex Miller [ 03/Nov/13 10:47 PM ]

Re-marking screened. Not sure what else to do.

Comment by Andy Fingerhut [ 04/Nov/13 8:35 AM ]

clj-1130-v2-ignore-ws.diff is identical to clj-1130-v2.diff, except it was produced with a command that ignores differences in a line due only to whitespace, i.e.: 'git format-patch master --stdout -w > clj-1130-v2-ignore-ws.diff'

It is not intended as the patch to be applied. It is only intended to make it easier to see that many of the lines in clj-1130-v2.diff are truly only differences in indentation.

Comment by Alex Miller [ 04/Nov/13 8:55 AM ]

Thanks Andy...

Comment by Rich Hickey [ 22/Nov/13 7:59 AM ]

This patch ignores the fact that method is checked for first above:

if(c != null)
  maybeField = Reflector.getMethods(c, 0, munge(sym.name), true).size() == 0;

Which is why the field code is unconditional. I'm fine with making errors better, but changing logic as well deserves more scrutiny.

Comment by Alex Miller [ 06/Dec/13 9:01 PM ]

This patch is intentionally trying to avoid calling StaticFieldExpr in the field code as that is where the (Long/parseLong) case (erroneously calling an n-arity static method with 0 args) will throw a field-oriented exception instead of a method-oriented exception. By adding the extra check here, this case falls through into the method case and throws later on calling StaticMethodExpr instead.

The early check is a check for methods of the specified arity. The later check is for the existence of a field of matching name. Combined, they lead to a better error message.

However, another alternative is to set maybeField in the first check based on field existence, not on invocation arity. That just improves the maybeField informaiton and the existing code then naturally throws the correct exception (and the patch is much simpler).

The similar case for calling n-arity instance methods with 0-arity has the same problem for the same reason:

user=> (.setTime (java.util.Date.))
IllegalArgumentException No matching field found: setTime for class java.util.Date  clojure.lang.Reflector.getInstanceField (Reflector.java:271)

Thus we can also adjust the other call that sets maybeField (which now is much less maybe).

I will attach a patch that covers these cases and update the ticket for someone to screen.

Comment by Stuart Sierra [ 08/Dec/13 12:24 PM ]

Screened. The patch clj-1130-v3.diff works as advertised.

This patch only improves error messages for cases when the type of the
target object is known to the compiler. In reflective calls, the error
messages are still the same.

Example, after this patch, given these definitions:

(def v 42)
(defn untagged-f [] 42)
(defn ^Long tagged-f [] 42)

The following expressions produce new error messages:

(.foo v 1)
;; IllegalArgumentException No matching method found: foo taking 1 args
;; for class java.lang.Long clojure.lang.Reflector.invokeMatchingMethod
;; (Reflector.java:53)

(.foo (tagged-f))
;; IllegalArgumentException No matching method found: foo taking 0 args
;; for class java.lang.Long clojure.lang.Reflector.invokeMatchingMethod
;; (Reflector.java:53)

These expressions still use the old error messages:

(.foo v)
;; IllegalArgumentException No matching field found: foo for class
;; java.lang.Long clojure.lang.Reflector.getInstanceField
;; (Reflector.java:271)

(.foo (untagged-f))
;; IllegalArgumentException No matching field found: foo for class
;; java.lang.Long clojure.lang.Reflector.getInstanceField
;; (Reflector.java:271)
Comment by Rich Hickey [ 03/Jan/14 8:41 AM ]

Changing the logic to get a different error message is something that needs to be done with great care. This now seems to prefer fields over methods, changing the semantics.

Comment by Stuart Halloway [ 31/Jan/14 3:12 PM ]

v4 patch simply enhances error messaages

Comment by Andy Fingerhut [ 31/Jan/14 3:18 PM ]

clj-1130-v4.diff has the same patch repeated twice in the file. clj-1130-v5.diff is identical, except deleting the redundant copy.





[CLJ-1102] Better handling of exceptions with empty stack traces Created: 04/Nov/12  Updated: 25/Oct/13  Resolved: 25/Oct/13

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: errormsgs

Attachments: File clj-1102-improve-empty-stack-trace-handling-v2.diff     Text File clj-1102-improve-empty-stack-trace-handling-v2.txt    
Patch: Code and Test
Approval: Ok

 Description   

REPL session demonstrating clojure.stacktrace/print-stack-trace and clojure.test/file-and-line throwing exceptions when given Throwable with an empty stack trace:

user=> (require '[clojure.stacktrace :as s])
nil
user=> (def empty-stack (into-array (Class/forName "java.lang.StackTraceElement") []))
#'user/empty-stack
user=> (def t (doto (Throwable.) (.setStackTrace empty-stack)))
#'user/t
user=> (def msg (with-out-str (s/print-stack-trace t)))

NullPointerException   clojure.lang.Reflector.invokeNoArgInstanceMember (Reflector.java:296)
user=> msg
#<Unbound Unbound: #'user/msg>
user=> (require 'clojure.test)
nil
user=> (def m1 (#'clojure.test/file-and-line t 0))

ArrayIndexOutOfBoundsException   java.lang.reflect.Array.get (Array.java:-2)
user=> m1
#<Unbound Unbound: #'user/m1>

I have seen this cause confusing output when exceptions with empty stack traces are thrown while running tests on a project. According to the Java docs for Throwable, it is permissible for getStackTrace to do this:

http://docs.oracle.com/javase/6/docs/api/java/lang/Throwable.html#getStackTrace%28%29

Approach:

I found all places in the Clojure code that call getStackTrace. Among them, two did not handle an empty stack trace correctly.

Output of tests above with this patch applied:

...

user=> (def msg (with-out-str (s/print-stack-trace t)))
#'user/msg
user=> (print msg)
java.lang.Exception: null
 at [empty stack trace]
nil

...

user=> (def m1 (#'clojure.test/file-and-line t 0))
#'user/m1
user=> m1
{:line nil, :file nil}

Patch: clj-1102-improve-empty-stack-trace-handling-v2.diff

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 04/Nov/12 5:07 PM ]

clj-1102-improve-empty-stack-trace-handling-v1.txt dated Nov 4 2012 improves the handling of .getStackTrace returning a length 0 array in two places. I checked all other places .getStackTrace was called and they seem to already handle this case gracefully.

Comment by Timothy Baldridge [ 30/Nov/12 2:57 PM ]

Vetting.





[CLJ-1083] Incorrect ArityException message for function names containing -> Created: 09/Oct/12  Updated: 25/Oct/13  Resolved: 25/Oct/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2, Release 1.3, Release 1.4, Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Alex Nixon Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: errormsgs

Attachments: File better-throw-arity-messages.diff     Text File clj-1083-better-throw-arity-messages-patch-v3.txt     Text File clj-1083-better-throw-arity-messages-patch-v5.txt     File clj-1083-better-throw-arity-messages-patch-v6.diff     Text File clj-1083-better-throw-arity-messages-patch-v6.txt     File clj-1083-better-throw-arity-messages-patch-v7.diff    
Patch: Code and Test
Approval: Ok

 Description   

Problem: Error messages show munged symbol names.

user=> (defn a->b [])
#'user/a->b
user=> (a->b 1)
ArityException Wrong number of args (1) passed to: user$a  clojure.lang.AFn.throwArity (AFn.java:437)

Note that the reported function name in the stack trace is "user$a", where it should be "user$a->b" (or some mangled variant thereof?)

Patch: clj-1083-better-throw-arity-messages-patch-v7.diff

Approach:

Demunge the name to print a better exception message. Note: demunging is not reversible if the original symbol contains a munged word (GT, LT, PLUS, etc). These cases are rare in actual code.

To avoid introducing a dependency on Clojure code from Java code, add new demunge() method to class Compiler, near the existing munge() method. Also replace the two existing Clojure implementations of demunge with a call to this new Java demunge(). Test contains both a couple example tests and a generative test for name munge/demunge roundtrip.

See discussion here: https://groups.google.com/d/msg/clojure/PVNoLclhhB0/_NWqyE0cPAUJ

Screened by: Alex Miller



 Comments   
Comment by Timothy Baldridge [ 26/Nov/12 10:27 AM ]

Fix for this defect.

Comment by Timothy Baldridge [ 26/Nov/12 10:30 AM ]

The throwArity now attempts to locate and call clojure.main/demunge. If it finds the function it invokes it and uses the returned string in the error. Otherwise it just throws the actual class name. This results in the following behaviour:

user=> (defn a->b [])
#'user/a->b
user=> (a->b 32)
ArityException Wrong number of args (1) passed to: user/a->b clojure.lang.AFn.throwArity (AFn.java:449)
user=>

Comment by Stuart Halloway [ 24/May/13 11:35 AM ]

Timothy: Why the empty catch block? I don't see anything in the try block whose failure we would want to ignore.

Comment by Timothy Baldridge [ 30/May/13 2:02 PM ]

The demunge function is created inside of a .clj file. So it is possible that we could hit this exception before demunge exists. The try simply says "if we can get a better error message, use it. Otherwise, fall back to the old (half-broken) method of getting method names"

Comment by Andy Fingerhut [ 07/Jun/13 2:01 PM ]

Presumptuously changing approval from Incomplete back to its former value of Vetted after Timothy responded to what I believe is the reason it was marked Incomplete (Stu's question).

Comment by Alex Miller [ 02/Jul/13 6:47 PM ]

There are some unnecessary whitespace changes in the patch.

Is it ok to add a dependency from AFn (even if an optional one) to clojure.main/demunge? (The same code is repeated in clojure.repl/demunge btw.) Seems like it would be better for something more common to have a demunge function that all of these could call.

Shouldn't we have a test in the patch?

Changing back to Incomplete.

Comment by Andy Fingerhut [ 23/Aug/13 12:44 PM ]

Is Rich the only person that can authoritatively answer Alex's question "Is it ok to add a dependency from AFn (even if an optional one) to clojure.main/demunge?"

I am not sure, but it seems the only way to avoid a dependency from AFn to Clojure code in this case would be to write a version of demunge in Java.

Comment by Andy Fingerhut [ 23/Aug/13 1:19 PM ]

Patch clj-1083-better-throw-arity-messages-patch-v2.txt dated Aug 23 2013 includes all of Timothy Baldridge's patch better-throw-arity-messages.diff dated Nov 26 2012 except it leaves out the unnecessary whitespace changes. It also adds a new test that fails without his patch, and passes with it.

It still has a dependency from AFn to clojure.main/demunge, so if that is not acceptable, something else will be needed.

Comment by Andy Fingerhut [ 23/Aug/13 7:52 PM ]

Patch clj-1083-better-throw-arity-messages-patch-v3.txt dated Aug 23 2013 includes all of Timothy Baldridge's patch better-throw-arity-messages.diff dated Nov 26 2012 except it leaves out the unnecessary whitespace changes. It also adds a new test that fails without his patch, and passes with it, and fixes a bug with both copies of the demunge implementation that caused it to transform some munged names incorrectly.

It still has a dependency from AFn to clojure.main/demunge, so if that is not acceptable, something else will be needed.

Comment by Alex Miller [ 26/Aug/13 5:23 PM ]

I still think AFn calling into clojure.main/demunge is weird.

Some other alternatives:

1) Move demunge to clojure.stacktrace (which is also not demunging). Still weird to be calling from the Java portions into the Clojure portions (this is not done elsewhere).

2) Another alternative would be to implement demunge() in another class (putting it in Compiler introduces another weird cycle), perhaps in Util?

Comment by Andy Fingerhut [ 26/Aug/13 7:44 PM ]

Alex, sorry if I'm being a bit dense here, but how does adding a Java version of demunge() into class Compiler introduce a cycle of dependencies? I think all Java source files are compiled before any Clojure source files, so I am guessing you are referring to a dependency between different Java classes? To be clear, when I suggested writing a Java version of demunge(), I mean a "pure Java" implementation that does not refer to anything in the Clojure source files.

My main reason for suggesting adding it to class Compiler is simply to keep the related munge() and proposed new demunge() methods next to each other, and the fields they use.

Comment by Alex Miller [ 27/Aug/13 6:33 AM ]

Sorry, I was thinking that it might be weird for AFn to depend on Compiler, but maybe that doesn't matter. Putting it next to munge() makes a lot of sense.

Comment by Andy Fingerhut [ 27/Aug/13 8:33 AM ]

Patch clj-1083-better-throw-arity-messages-patch-v4.txt dated Aug 27 2013 adds a Java implementation of a demunge method to the Compiler class, and uses it in place of previously-existing Clojure implementations of demunge, as well as in the code for throwing arity exceptions.

It introduce a dependency on the Compiler class from class AFn, but not any dependencies on Clojure code from Java code.

Comment by Andy Fingerhut [ 27/Aug/13 9:54 AM ]

Patch clj-1083-better-throw-arity-messages-patch-v5.txt dated Aug 27 2013 is identical to previously attached -v4 described above, except it leaves out an unintentional change to the duration of Clojure's generative tests.

Comment by Alex Miller [ 02/Sep/13 10:43 PM ]

I reworked Andy's patch to clean up the Java code in Compiler. The logic seems correct. I added a generative test that checks for roundtripping a symbol name through munge/demunge. The only tricky part of that is avoiding symbol names which cannot properly roundtrip.

Marking screened.

Comment by Andy Fingerhut [ 03/Sep/13 6:24 AM ]

Thanks, Alex. I don't want to derail all of our work because of what is described in this comment, but wanted to record it somewhere after tracking it down.

The fact that demunge as it existed before this patch (and as it still behaves after this patch), can produce names that are not the original function name makes me finally realize why Chas Emerick made this comment in the email thread linked in the description:

"This is perhaps another case where pushing var metadata (or some subset thereof) down to the function being defined in defns would be beneficial; AFunction could then override throwArity to just use the :name of the function being called, thus avoiding any confusion introduced by munged or un-munging names."

It seems that if Chas's idea were implemented, it could be used to improve the output of throwArity to give the correct original Clojure function name in all cases. However, even then the demunge'ing of StackTraceElement class names could not be similarly improved (e.g. in the output of clojure.repl/pst), because all they contain are class names as strings, so functions b? and b_QMARK_ will always appear identical in a StackTraceElement.

Given all of that, I'd say take the best patch for this ticket, and simply advise people not to use substrings like "QMARK" or "COLON" in their function names. If they do, they should be prepared for confusion due to demunge'ing when examining stack traces with functions like clojure.repl/pst

Comment by Alex Miller [ 03/Sep/13 7:47 AM ]

I think the current approach is fine. Munge never made any promises about being reversible.

Given typical naming styles in Clojure, this should rarely be an issue. The consequence is that now you'll get a wrongly demunged (instead of wrongly munged) version; in either case you're not getting the original function name. I enjoyed discovering this detail via the generative tests!!

I do agree that having some way to recover the original function name would be a better solution.

Comment by Andy Fingerhut [ 25/Oct/13 12:25 PM ]

I've not looked into details yet, but screened patch clj-1083-better-throw-arity-messages-patch-v6.diff fails to apply cleanly as of Oct 25, 2013 latest Clojure master.

Comment by Alex Miller [ 25/Oct/13 1:12 PM ]

Updated patch to apply cleanly to master as of Oct 25, 2013.





[CLJ-1056] defprotocol: invalid method overload syntax getting accepted Created: 01/Sep/12  Updated: 25/Oct/13  Resolved: 25/Oct/13

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

Type: Defect Priority: Minor
Reporter: Víctor M. Valenzuela Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: errormsgs, protocols

Attachments: Text File clj-1056-1.txt     File clj-1056-2.diff     Text File clj-1056-2.txt    
Patch: Code and Test
Approval: Ok

 Description   

The compiler accepts this erroneous form:

user=> (defprotocol Bar (m [this]) (m [this arg]))
Bar

Analysis: defprotocol silently assoc's the last list of signatures found for any particular method name, without checking whether the method name was given earlier.

Patch: clj-1056-2.diff

Approach: Modify defprotocol to check whether each method name has already been encountered earlier, and throw an exception if so. The patch also updates the error message for the case of a protocol function with no args specified.

Behavior with patch clj-1056-2.txt:

user=> (defprotocol Bar (m [this]) (m [this arg]))
IllegalArgumentException Function m in protocol Bar was redefined. Specify all arities in single definition.
user=> (defprotocol Foo (m []))
IllegalArgumentException Definition of function m in protocol Foo must take at least one arg.

Screened by: Stuart Halloway



 Comments   
Comment by Timothy Baldridge [ 29/Nov/12 4:02 PM ]

Can not reproduce the fist error:

user=> (defprotocol Foo (f ([this]) ([this arg])))
CompilerException java.lang.IllegalArgumentException: Parameter declaration missing, compiling:(NO_SOURCE_PATH:5:1)

But the 2nd one I can reproduce:

user=> (defprotocol Bar (m [this]) (m [this arg]))
Bar
user=> Bar
{:on-interface user.Bar, :on user.Bar, :sigs {:m {:doc nil, :arglists ([this arg]), :name m}}, :var #'user/Bar, :method-map {:m :m}, :method-builders {#'user/m #<user$eval71$fn_72 user$eval71$fn_72@1a2b53fb>}}
user=>

Notice that :arglists only has one entry

Vetting

Comment by Alex Miller [ 22/Aug/13 10:36 PM ]

Moving back to Triaged as Rich has not vetted.

Comment by Andy Fingerhut [ 10/Sep/13 11:24 PM ]

Patch clj-1056-1.txt changes defprotocol to throw an exception if the same method name appears more than once.

Without this patch, the last set of signatures for a method name "wins", silently overriding any earlier ones.

Comment by Alex Miller [ 18/Oct/13 11:08 AM ]

Updated patch to improve error messages and to switch from CompilerException (which is not used outside the Compiler) back to IllegalArgumentException.

I think it would be a good idea to have a general Exception that could carry file/line/col info that could be used by Compiler but also other errors (the EdnReader and LispReader both have their own variants). But I think that should be a separate enhancement, which I have filed as CLJ-1280.





[CLJ-1030] Misleading ClassCastException when coercing a String to int Created: 25/Jul/12  Updated: 05/Sep/13

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

Type: Enhancement Priority: Minor
Reporter: Philipp Meier Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

Attachments: File improved-int-char-casting-error-messages.diff     File string-coerce-to-int.diff    
Patch: Code and Test

 Description   

Observed behaviour

(int "0") =>
java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Character

Expected behaviour
(int "0") =>
java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Integer
or 
IllegalArgumentException


 Comments   
Comment by Andy Fingerhut [ 24/Sep/12 11:20 AM ]

If someone wants to improve the behavior of int in this case, they should also consider similar improvements to the error messages for unchecked-int, char, and unchecked-char.

Comment by Michael Drogalis [ 06/Jan/13 8:45 PM ]

Patch improved-int-char-casting-error-messages.diff on January 6, 2013.

Comment by Alex Miller [ 03/Sep/13 12:04 PM ]

int knows how to coerce numbers and chars to ints. It does not currently support Strings. It would be worthwhile to either add this behavior or make int's docstring more descriptive than just "Coerce to int".

Comment by Michael Drogalis [ 04/Sep/13 8:25 AM ]

string-coerce-to-int.diff
September 4th, 2013
Fixes int and char casting error messages, and allows casting from String to int as Alex Miller suggested.

Comment by Michael Drogalis [ 05/Sep/13 12:39 PM ]

Notably, (map int ["1" "2" "3"]) fails with "ClassCastException java.lang.String cannot be cast to java.lang.Character" using this patch.
It emanates from RT.java from intCast(Object). So it looks like the Strings are losing their types when they're being mapped over.
I would guess this is intentional?





[CLJ-969] Symbol/keyword implements IFn for lookup but a non-collection argument produces non-intuitive results Created: 09/Apr/12  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs


 Description   

('+ 1 2) ;; return 2 because it is treated as (get 1 '+ 2)

Whilst this is "consistent" once you know the lookup behavior, it's confusing for Clojure newbies and it seems to be a non-useful behavior.

Proposal: modify Keyword.invoke() and Symbol.invoke() to restrict first Object argument to instanceof ILookup, Map or IPersistentSet (or null) so that the "not found" behavior doesn't produce non-intuitive behavior.






[CLJ-939] Exceptions thrown in the top level ns form are reported without file or line number Created: 24/Feb/12  Updated: 22/Nov/13  Resolved: 22/Nov/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3, Release 1.4, Release 1.5, Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Hugo Duncan Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: errormsgs

Attachments: File 0001-report-load-exceptions-with-file-and-line.diff     File 0002-report-load-exceptions-with-file-and-line.diff     Text File clj-939-report-load-exceptions-with-file-and-line-patch-v2.txt     File clj-939-report-load-exceptions-with-file-and-line-patch-v3.diff     Text File clj-939-report-load-exceptions-with-file-and-line-patch-v3.txt     File clj-939-v4.diff     File screen-clj-939.org    
Patch: Code
Approval: Ok

 Description   

If there is an error in the `ns` form, an exception is thrown, which is not caught in `load`.

For example, with an invalid :only clause;

(ns clj14.myns
  (:use
   [clojure.core :only seq]))

With the latest Clojure master as of Aug 24 2013, this generates the following exception, with no source file or line number except the one shown in clojure.core:

Exception :only/:refer value must be a sequential collection of symbols  clojure.core/refer (core.clj:3854)

You can find a source file in your project if you painstakingly search through the stack trace, but it would be nice if it jumped out at you in the exception itself.

Patch: clj-939-v4.diff

Approach: The latest patch does not modify the behavior of any other exceptions thrown by the compiler. The throw-if function is only used in the load-related functions: this patch changes it to throw CompilerException instead of Exception. As a result, exceptions which occur while loading files will be decorated with file names and line/column numbers. The line/column numbers are not always accurate (see notes in attachment screen-clj-939.org) but the file names are correct.

This is an incremental improvement to compile-time error messages, but it does not solve some of the fundamental problems with reporting correct line/column numbers from errors thrown by the compiler.

Screened by: Stuart Sierra (re-screened by Alex for the specific comments by Rich)



 Comments   
Comment by Hugo Duncan [ 25/Feb/12 8:26 AM ]

Corrected patch

Comment by Andy Fingerhut [ 09/Mar/12 9:26 AM ]

Patch 0001-report-load-exception-with-file-and-line.diff fails build. Patch 0002-report-load-exception-with-file-and-line.diff applies, builds, and tests cleanly as of March 9, 2012. Hugo has signed a CA.

Comment by Andy Fingerhut [ 05/Oct/12 8:13 AM ]

clj-939-report-load-exceptions-with-file-and-line-patch-v2.txt dated Oct 5 2012 is intended to be an update to Hugo Duncan's patch 0002-report-load-exceptions-with-file-and-line.diff dated Feb 25 2012. Because of Brandon Bloom's recently commited patch adding column numbers in addition to line numbers, this is not simply updating some lines of context, but I think it is correct. It would be good if Hugo could take a look at it and confirm.

Comment by Stuart Sierra [ 09/Nov/12 9:38 AM ]

Screened.

The error messages are better than what we had before. The line/column numbers are not particularly informative, probably because ns is a macro.

Comment by Rich Hickey [ 13/Nov/12 3:37 PM ]

This patch doesn't change the reporting on any other (e.g. nested) exceptions? It looks like it might.

Comment by Alex Miller [ 28/Jul/13 10:12 PM ]

This issue has somewhat strange history. Starting over with Triaged so it can go through release assignment and patch process again.

Comment by Andy Fingerhut [ 25/Aug/13 6:01 AM ]

Patch clj-939-report-load-exceptions-with-file-and-line-patch-v3.txt dated Aug 25 2013 started based upon Hugo Duncan's
clj-939-report-load-exceptions-with-file-and-line-patch-v2.txt patch.

It was enhanced to avoid wrapping a CompilerException within another CompilerException first. That always gave line 1 and column 1 for all load/require/use exceptions, so I modified clojure.core/throw-if to throw a CompilerException instead. The line/column numbers with this approach are now usually those of the beginning of the enclosing ns form, if these operations are caused by an ns form.

I also added a little extra info to one exception message, and a new condition in which an exception is thrown if a require/use operand is neither a libspec nor a prefix list.

Comment by Stuart Sierra [ 27/Sep/13 9:45 AM ]

Screening in progress.

Comment by Stuart Sierra [ 27/Sep/13 12:57 PM ]

Screened. Notes in attachment screen-clj-939.org

Comment by Rich Hickey [ 25/Oct/13 7:06 AM ]

this (surreptitiously? adds a new test for vector or list. Please submit patches that address one specific issue. There's no reason for this concrete check, but if you think there is make another ticket.

Comment by Andy Fingerhut [ 25/Oct/13 9:06 AM ]

To: Operative Sierra
From: Chaos

Agent Hickey thwarted world domination plans yet again. Recommend plan omicron. InfoSec dept provided clj-939-v4.diff - should enable our insidious changes to slip past his Detect-O-Tron. Good luck, operative. Message ends.

Comment by Alex Miller [ 25/Oct/13 9:58 AM ]

Seems like the (declare list?) can go away too?

Comment by Andy Fingerhut [ 25/Oct/13 10:49 AM ]

Good catch, Alex. More coffee required here. Uploaded new clj-939-v4.diff in place of the previous one.

Comment by Alex Miller [ 25/Oct/13 10:59 AM ]

Verified questioned bits have been removed and patch still applies cleanly and tests pass. Re-marking Screened with new patch.





[CLJ-888] defprotocol should throw error when signatures include variable number of parameters Created: 29/Nov/11  Updated: 17/Aug/15

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

Type: Enhancement Priority: Major
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs, protocols

Attachments: Text File 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch     Text File CLJ-888-v2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

I tried to use & in the signature for a method in defprotocol. Apparently (see below), this is compiled so that & becomes a simple parameter name, and there is no special handling for variable number of parameters. I think the use of & in a protocol signature ought to be detected and immediately cause an exception (I also think this restriction on the signatures ought to be documented; I couldn't find it specified in the current documentation, though of course it is implied (as I later realized) by the fact that defprotocol creates a Java interface).

user=> (defprotocol Applier (app [this f & args]))
Applier
user=> (deftype A [] Applier (app [_ f & args] (prn f & args) (apply f args)))
user.A
user=> (app (A.) + 1 2)
#<core$PLUS clojure.core$PLUS@5d9d0d20> 1 2
IllegalArgumentException Don't know how to create ISeq from: java.lang.Long
clojure.lang.RT.seqFrom (RT.java:487)



 Comments   
Comment by Alex Coventry [ 21/Oct/13 4:21 PM ]

Patch with test code attached. I have it throwing a CompilerException so that it shows source code location. Not sure whether this is kosher in clojure code, but I wish more macros provided this in their error handling.

Comment by Tassilo Horn [ 22/Oct/13 6:26 AM ]

This issue has already been discussed in CLJ-1024. There I provided a patch that forbids varargs and destructuring forms at various places including defprotocol/definterface. My patch had been applied shortly before clojure 1.5 was released, but it had a bug (forbid too many uses), so it got reverted and the bug closed and declined.

I was told to bring up the issue again after 1.5 has been released.

So here is my patch again. This time it's much more relaxed and only forbids varargs in defprotocol/definterface method declarations, and in deftype/defrecord and reify method implementations.

Comment by Alex Coventry [ 22/Oct/13 7:30 AM ]

Thanks, Tassilo. If there's anywhere in the JIRA system where I could check for prior work like that for other similar issues, I'd be grateful for a pointer.

Best regards,
Alex

Comment by Tassilo Horn [ 22/Oct/13 7:39 AM ]

New version of my patch.

Now I use a CompilerException with proper file/line/column information like Alex did. I also added his test case (which passes).

Concerning your question, Alex: a search for "varargs" would have listed CLJ-1024, but probably you wouldn't have looked into it anyway, because it's a closed issue...

Comment by Tassilo Horn [ 22/Oct/13 7:44 AM ]

Alex, if you don't object could we remove your patch in favor of mine which covers a bit more cases?

Comment by Alex Coventry [ 22/Oct/13 10:57 AM ]

Yep. Just read through 1024 and the associated mailing list discussion. You should totally get the credit: Your patch is more comprehensive and you have been on this a long time. Thanks for folding in the good parts of my patch.

Best regards,
Alex

Comment by Tassilo Horn [ 22/Oct/13 12:15 PM ]

Ok, great.

It seems I don't have the permissions to delete other peoples' attachments, so could you please delete your patch yourself?

Comment by Alex Coventry [ 23/Oct/13 2:44 PM ]

Sure, Tassilo. It's done.

I think this also needs a regression test for the case hugod originally pointed out. I initially made the same mistake as you there, but amalloy pointed it out[1] before I submitted the patch, so it is a natural mistake to make and should probably be documented in the source code.

Best regards,
Alex

[1] http://logs.lazybot.org/irc.freenode.net/%23clojure/2013-10-21.txt search for 14:48:34.

Comment by Tassilo Horn [ 24/Oct/13 2:00 AM ]

Alex, I've added the regression test you suggested. Thanks for pointing that out.

Also, I added tests checking definterface method declarations, and tests checking inline method implementations made with defrecord, deftype, and reify.

However, there's a problem with the tests for deftype and reify I don't know how to fix. When I eval the macroexpand forms used in the tests in a REPL, I can see that the CompilerException is successfully thrown and printed. But it also seems to be caught somewhere in the middle, so that the macroexpand returns a form and the exception doesn't make it to the (is (thrown? ...)). Therefore, I've commented the these tests and added a big FIXME.

Comment by Tassilo Horn [ 24/Oct/13 2:28 AM ]

New version of the patch with now all tests uncommented and passing. Andy Fingerhut made me aware that for the 4 deftype and reify tests, I need eval instead of just macroexpand.

Comment by Andy Fingerhut [ 25/Oct/13 6:25 PM ]

I have not investigated the reason yet, but patch 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch no longer applies cleanly after the latest commits to Clojure master on Oct 25 2013.

Comment by Tassilo Horn [ 28/Oct/13 2:21 AM ]

I've rebased the patch onto the current master so that it applies cleanly again.

Comment by Tassilo Horn [ 28/Oct/13 2:25 AM ]

Stu, I've assigned this issue to you because you've been assigned to CLJ-1165 which I have closed as duplicate of this issue.

One minor difference between my patch to this issue and CLJ-1165 is that here I use a CompilerException with file/line/column info whereas in CLJ-1165 I've used `ex-info`. I think the CE is more appropriate/informative, as the error is already triggered during macro expansion.

Comment by Michael Blume [ 16/Aug/15 4:36 PM ]

Rebased onto master

Comment by Michael Blume [ 17/Aug/15 12:46 PM ]

Be nice if we got this merged – I just got a pull request using a varargs protocol that seemed to work by accident because the only time the varargs arity was called, it was called with two additional arguments, matching the '& and the 'args in the interface. Confused the hell out of me before I worked out what was going on.





[CLJ-884] Reflector error messages can be improved when no matching method is found. Created: 27/Nov/11  Updated: 18/Apr/14  Resolved: 18/Apr/14

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

Type: Enhancement Priority: Major
Reporter: Rahul Pilani Assignee: Unassigned
Resolution: Duplicate Votes: 1
Labels: errormsgs
Environment:

All


Attachments: Text File diff.patch    
Patch: Code

 Description   

When accessing a java method with an arity mismatch or a mismatched parameter type, Reflector.java returns the following error on REPL:
IllegalArgumentException No matching method found: xyz for class com.abc.MyClass

eventhough method xyz might exist on MyClass, but was being called with the wrong number of arguments.

Attached is a patch that fixes that problem.



 Comments   
Comment by Andy Fingerhut [ 22/Mar/12 8:47 PM ]

diff.patch of Nov 27, 2011 does not apply cleanly to latest master version of Clojure code (using "patch -p1 < diff.patch", at least). It is preferred by Clojure team that patches are in git format-patch format. Instructions for producing such a patch are given at http://clojure.org/patches

Rahul, are you planning to sign a Clojure Contributor Agreement? Without that, this code cannot be included in Clojure, unless a contributor reimplements it on their own.

Comment by Andy Fingerhut [ 23/Mar/12 1:14 AM ]

In private communication with the patch author today, he expressed an interest in submitting a signed CA so this patch can be considered for inclusion in Clojure.

Comment by Kevin Downey [ 17/Apr/14 10:48 PM ]

it has been two years, is there a CA to go with this patch yet?

Comment by Andy Fingerhut [ 18/Apr/14 2:01 AM ]

The patch author has not submitted a CA – their name is not listed at http://clojure.org/contributing

Everyone else is free to submit a patch if they wish.

Comment by Alex Miller [ 18/Apr/14 7:40 AM ]

I think CLJ-1130 is the same issue and it is much farther along in the process.





[CLJ-790] Primitive type hints on function names should print error message Created: 10/May/11  Updated: 23/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Alan Dipert Assignee: Alan Dipert
Resolution: Unresolved Votes: 0
Labels: errormsgs


 Description   

Functions returning primitives are hinted with metadata on the argument list, not on the function name. Using a primitive type hint on a function name should print an error message.

Currently, misplaced primitive hints are read without error.



 Comments   
Comment by Andy Fingerhut [ 23/Feb/15 9:20 PM ]

One can type hint a primitive value on a Var naming a function, or any value one wants, like so:

(def {:tag 'long} foo 17)

(defn {:tag 'double} bar [x y]
(* 2.0 x y))

I think it is odd that one must use {:tag 'long} instead of ^long, since trying to use ^long ends up giving the useless type hint that is the value of the function clojure.core/long.

However, the Clojure compiler will use the primitive type hints as shown in the examples above to avoid reflection in appropriate Java interop calls, so making them an error seems undesirable.





[CLJ-735] Improve error message when a protocol method is not found Created: 04/Feb/11  Updated: 04/May/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: errormsgs

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

 Description   

If you call a protocol function but pass the wrong arity (forget an argument for example), you currently a message that says "No single method ... of interface ... found for function ... of protocol ...". The code in question is getting matching methods from the Reflector and creates this message if the number of matches != 1.

There are really two cases there:

  • matches == 0 - this happens frequently due to typos
  • matches > 1 - this presumably happens infrequently

I propose that the == 0 case instead should have slightly different text at the beginning and a hint as to the intended arity within it:

"No method: ... of interface ... with arity ... found for function ... of protocol ...".

The >1 case should have similar changes: "Multiple methods: ... of interface ... with arity ... found for function ... of protocol ...".

Patch is attached. I used case which presumably should have better performance than a nested if/else. I was not sure whether the reported arity should match the actual Java method arity or Clojure protocol function arity (including the target). I did the former.

I did not add a test as I wasn't sure whether checking error messages in tests was appropriate or not. Happy to add that if requested.



 Comments   
Comment by Chas Emerick [ 14/Jul/11 6:39 AM ]

I was not sure whether the reported arity should match the actual Java method arity or Clojure protocol function arity (including the target). I did the former.

I think it should be the latter. The message is emitted when the protocol methods are being invoked through the corresponding function, so it should be consistent with the errors emitted by regular functions.

+1 for some tests, too. There certainly are tests for reflection warnings and such.

FWIW, I'm happy to take this on if Alex is otherwise occupied.

Comment by Alex Miller [ 04/May/15 4:22 PM ]

Picking this up a zillion years later ...

1) I think I would now change the control flow slightly to make the normal path (methods.size() == 1) first and the error cases after that without that switch/case stuff:

if(methods.size() == 1) { 
  this.onMethod = (java.lang.reflect.Method) methods.get(0);
} else if(methods.size() == 0) {
  ...
} else {
  ...
}

2) The Compiler code should use tabs instead of spaces.

3) I would like tests added for the error messages in these two cases. Those can go in clojure.test-clojure.protocols.





[CLJ-706] make use of deprecated namespaces/vars easier to spot Created: 05/Jan/11  Updated: 23/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs

Attachments: File 706-deprecated-var-warning.diff     Text File 706-deprecated-var-warning-patch-v2.txt     File 706-fix-deprecation-warnings-agents.diff     File 706-fix-deprecation-warnings-on-replicate.diff     File 706-fix-deprecation-warning-test-junit.diff     File 706-warning-on-deprecated-ns.diff    
Patch: Code

 Description   

From the mailing list http://groups.google.com/group/clojure/msg/c41d909bd58e4534. It is easy to use deprecated namespaces without knowing you are doing so. The documentation warnings are small, and there is no compiler warning.

Some possibilities include:

  1. much more visible deprecation styling in the documentation
  2. stderr warnings when referencing a deprecated thing.

I don't love the idea of stderr warnings on all the time. Rich: is there an approach to this that you would like to see a patch for?



 Comments   
Comment by Rich Hickey [ 07/Jan/11 9:38 AM ]

I don't mind warning to stderr

Comment by Luke VanderHart [ 26/Oct/12 1:37 PM ]

706-deprecated-var-warning.diff adds warnings when using deprecated vars. The other three patches clean up deprecation warnings.

Comment by Andy Fingerhut [ 26/Oct/12 2:23 PM ]

Great stuff. I looked through the first patch, and didn't see anything in there that lets someone disable deprecation warnings from the command line, the way that warn-on-reflection can today be set to true with a command line option.

Is that something important to have for deprecation warnings?

Comment by Andy Fingerhut [ 28/Oct/12 4:57 PM ]

I was hoping it would be quick and easy to add source file, line, and column info to the deprecation warning messages. It isn't as easy as adding them to the format() call, because the method analyzeSymbol doesn't receive these values as args. Is this deprecation check being done in a place where it is not easy to relate it to the source file, line, and column? Could it be done in a place where that info is easily available?

Comment by Ghadi Shayban [ 29/Oct/12 1:02 AM ]

Another patch - this time to warn on loading deprecated namespaces, instead of vars. This patch requires the first one.

Re: line/column, I'll figure out how to thread the compile context through if it's desired.

Re: Compile flag. I have a patch for this also, but I'm still verifying how to invoke. How is warn-on-reflection set by command line?

Comment by Andy Fingerhut [ 29/Oct/12 1:43 AM ]

Re: Compile flag. Don't hold off on implementing a flag if you want to, but it might be worth hearing from others whether such a command line option is even desired. I was asking in hopes of eliciting such a response.

For the way that it is handled in the Clojure compiler, search for REFLECTION_WARNING_PROP and related code in Compile.java. If you invoke the Clojure compiler directly via a Java command line, use -Dclojure.compile.warn-on-reflection=true (default is false). See the recent email thread sent to Clojure Dev Google group if you want to know how to do it via ant or Maven. Link: https://mail.google.com/mail/?shva=1#label/clojure-dev/13aa0e34530196c3

There is also a separate command-line flag called compiler-options (see Compile.java) that is implemented as a map inside the compiler. It was added more recently than warn-on-reflection, and might be the preferred method to add more such options, to avoid having to continue to add more arguments to the pushThreadBindings calls done in several places.

Comment by Ghadi Shayban [ 29/Oct/12 10:36 AM ]

Thanks, Andy.

Alternately for the last ns patch, it is equivalent to call (print-method msg err), rather than binding out to err, may be more readable. I'll be glad to send that in if it's preferable.

Comment by Andy Fingerhut [ 13/Feb/13 12:38 AM ]

706-deprecated-var-warning-patch-v2.txt dated Feb 12 2013 is identical to 706-deprecated-var-warning.diff dated Oct 26 2012, except it applies cleanly to latest master.

Comment by Andy Fingerhut [ 23/Feb/15 8:21 PM ]

For the information of anyone examining this ticket wishing for this feature, the Eastwood lint tool reports calls to deprecated Clojure functions, and also to deprecated Java methods. https://github.com/jonase/eastwood





[CLJ-420] Undefined symbols raise exceptions with line/column number of enclosing expression Created: 08/Aug/10  Updated: 05/Jun/15  Resolved: 25/Oct/13

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

Type: Defect Priority: Minor
Reporter: Alexander Redington Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: errormsgs, reader

Attachments: Text File CLJ-420-2.patch     Text File CLJ-420-3.patch     Text File CLJ-420-4.patch     Text File CLJ-420.patch    
Patch: Code
Approval: Screened

 Description   

Certain kinds of errors in loaded source files are coming back tagged with the correct source file, but an incorrect line:column number. This seems to happen when unknown symbols occur by themselves, not called as a function.

The general pattern appears to be that an undefined symbol is reported with a line number of the beginning of its nearest enclosing expression. If the undefined symbol appears at the top level of a file, it is reported with line:column number 0:0, or line:column number of REPL input, if loaded from a REPL. The behavior is different in a Leiningen REPL. If the undefined symbol appears at the top level of a file, it is reported with line:column number 1:1.

$ cat test1.clj 

bla
$ cat test2.clj 

(bla)
$ java -cp ../../opt/clojure/clojure-1.5.1.jar:. clojure.main
Clojure 1.5.1
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:1:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:5:1)

Patch: CLJ-420-3.patch

Approach: Capture line and column metadata for symbols in the LispReader. A few tests were adjusted to ignore line and col metadata for protocol symbols which now have them.

Screened by: Alex Miller

Background: Clojure Google group thread when this issue was originally reported in 2010 against Clojure 1.2: http://groups.google.com/group/clojure/browse_frm/thread/beb36e7228eabd69/a7ef16dcc45834bc?hl=en#a7ef16dcc45834bc



 Comments   
Comment by Assembla Importer [ 28/Sep/10 9:59 PM ]

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

Comment by Assembla Importer [ 28/Sep/10 9:59 PM ]

stu said: Updating tickets (#427, #426, #421, #420, #397)

Comment by Assembla Importer [ 28/Sep/10 9:59 PM ]

stu said: Updating tickets (#429, #437, #397, #420)

Comment by Alex Miller [ 10/Aug/13 12:20 AM ]

Based on the updated information (which is really a totally different issue), I have reduced priority from Major to Minor, removed the fix version and sent this back down to Triaged for Rich to take another look.

Comment by Paavo Parkkinen [ 28/Sep/13 6:58 AM ]

Just noticed that when I reproduce this with current code from Github, I get the same behaviour that was in the original report.

$ cat test1.clj 

bla
$ cat test2.clj 

(bla)
$ java -cp target/clojure-1.6.0-master-SNAPSHOT.jar:. clojure.main
Clojure 1.6.0-master-SNAPSHOT
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:1:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:2:1) 
user=> (require 'test2)    
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:5:1) 
user=> 
Comment by Paavo Parkkinen [ 28/Sep/13 7:23 AM ]

I also get the original behaviour with 1.5.1.

$ java -cp ../../opt/clojure/clojure-1.5.1.jar:. clojure.main
Clojure 1.5.1
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:1:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:5:1) 
user=> 

Could be lein is mixing it up. I also get the behaviour in the description if I do it with lein.

Comment by Andy Fingerhut [ 28/Sep/13 11:22 PM ]

Paavo, I think you are correct that I was getting different behavior than the original description because I was using Leiningen, rather than straight Clojure, and that the original description's behavior is still true with the latest Clojure master when Leiningen is not used. My bad for changing the description unnecessarily. Would you be willing to correct it?

Comment by Paavo Parkkinen [ 29/Sep/13 7:21 AM ]

Corrected description.

Comment by Paavo Parkkinen [ 06/Oct/13 5:20 AM ]

I have a fix for This bug. I will attach it to the ticket, but it is not supposed to be considered for approval, as it is clearly not finished (i.e. cleaned up) yet. If there is a better way to solicit comments for a proposed fix, please let me know.

For the fix, I simply copied the line:column number tracking code from ListReader.invoke (also used in others) into LispReader.read for Symbols. Only issues I see is that some test cases get confused because some meta data contains line:column info where it didn't use to.

I also tried to fix the line:column numbering for files like test3 and test4 below, and the fix is in the patch file, but commented out, because the fix for that causes some compile problems. I have not yet tracked down what the cause of the problems is. I suppose the correct procedure is to leave it out for now, and possibly open a new ticket for it?

$ cat test3.clj 

(
bla)
$ cat test4.clj 

(print
bla)

If anyone has any comments or suggestions, I would be very happy to hear them. In the mean time I'll start checking out the failing test cases, and seeing if I can correct them to work with the fix. I'm not sure how I should go about testing the fix itself.

Also, if the comments here is not the correct place for this discussion, please let me know and I will move it elsewhere (clojure-dev).

P.s. I suppose for test2, the column number should be 2, not 1. Not sure if that's a big deal or not.

Comment by Alex Miller [ 17/Oct/13 11:04 PM ]

A few things here:

1) I think you should drop the commented Compiler changes and just focus on the reader changes.
2) This patch has a number of test failures. In particular, protocol symbols now have line and column metadata. That's potentially quite useful but I haven't fully contemplated the ramifications.

Moving to Incomplete for now.

Comment by Paavo Parkkinen [ 19/Oct/13 9:40 PM ]

I attached a new version of patch, which gets rid of the Compiler changes, and fixes the test cases by simply ignoring the additional line/column metadata. I'm not aware of any way to reliably know what the values are going to be so they could be checked.

Or course there might be code that relies on the metadata not including line/column info, and that code will break with the patch. I can't really think of any reason why anyone would do that, and can't estimate how many people might be affected. After the patch people will probably start using the new metadata, and after that it will be difficult to get rid of, if needed in the future.

There is, I think, also potential performance effects. I believe the metadata is generated at compile time, but will be available at runtime, which will mean at least a minor increase in footprint, and possibly CPU as well. Again, I'm not that familiar with how the runtime works that I could really estimate that.

Of course there may also be other effects that I can't imagine right now.

Comment by Alex Miller [ 20/Oct/13 5:05 PM ]

I updated with a new patch that fixes some whitespace errors and removes the CLJ-420 comments which don't seem very helpful to me. Marking screened.

Comment by Rich Hickey [ 25/Oct/13 7:51 AM ]

this breaks tests, and will also break a ton of code not expecting this new metadata

Comment by Paavo Parkkinen [ 28/Oct/13 7:44 AM ]

Attached a patch (CLJ-420-4.patch) which fixes this without introducing the new metadata into symbols. Instead of using metadata to pass the line/column info from the reader to the compiler, I just store it in fields in the Symbol class.

Comment by Andy Fingerhut [ 28/Oct/13 1:31 PM ]

Paavo, I am not a Clojure screener, and cannot say any of this with authority to back it up, but Rich chose to close this ticket as declined, meaning that it would take some convincing for him to consider a ticket for it again. I am not saying you can't attach all of the patches you want to this ticket, but they might not go anywhere.

Comment by Alex Miller [ 28/Oct/13 2:33 PM ]

+1 to Andy's comment.

Further, I don't think the replacement patch respects the immutability and immutability concerns of interned Symbols - setting the line and column is a big race condition in the patch as is. IF this path were going to be acceptable, line and column would need to be final fields set in the Symbol constructor. However, it seems to me that you could easily have two symbols sharing the same interned Symbol that perhaps were created in different locations.

An additional item to consider is the additional memory overhead of carrying two additional ints on every Symbol. In general, it does not seem to me that this path provides enough value per overhead/effort so I would think I would recommend letting it go OR filing a new ticket.

Comment by Paavo Parkkinen [ 28/Oct/13 7:10 PM ]

I didn't realize the ticket was closed. Thanks for pointing that out.

And thanks for taking the time to still take a look at and comment on my patch, Alex. It's good advice.

Comment by Nicola Mometto [ 05/Jun/15 9:55 AM ]

Ticket was declined and marked as resolved but not closed. I'm closing it.





[CLJ-415] smarter assert (prints locals) Created: 29/Jul/10  Updated: 06/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs

Attachments: Text File clj-415-assert-prints-locals-v1.txt     Text File CLJ-415-v2.patch    
Patch: Code
Approval: Vetted
Waiting On: Rich Hickey

 Description   

Here is an implementation you can paste into a repl. Feedback wanted:

(defn ^{:private true} local-bindings
  "Produces a map of the names of local bindings to their values."
  [env]
  (let [symbols (map key env)]
    (zipmap (map (fn [sym] `(quote ~sym)) symbols) symbols)))

(defmacro assert
  "Evaluates expr and throws an exception if it does not evaluate to
 logical true."
  {:added "1.0"}
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep# (System/getProperty "line.separator")]
           (throw (AssertionError. (apply str "Assert failed: " (pr-str '~x) sep#
                                          (map (fn [[k# v#]] (str "\t" k# " : " v# sep#)) ~bindings)))))))))


 Comments   
Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

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

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

alexdmiller said: A simple example I tried for illustration:

user=> (let [a 1 b 2] (assert (= a b)))
#<CompilerException java.lang.AssertionError: Assert failed: (= a b)
 a : 1
 b : 2
Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

fogus said: Of course it's weird if you do something like:

(let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= x y)))
java.lang.AssertionError: Assert failed: (= x y)
 x : 1
 y : 2
 z : 3
 a : 1
 b : 2
 c : 3
 (NO_SOURCE_FILE:0)
</code></pre>

So maybe it could be slightly changed to:
<pre><code>(defmacro assert
  "Evaluates expr and throws an exception if it does not evaluate to logical true."
  {:added "1.0"}
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                          (map (fn [[k# v#]] 
                                                 (when (some #{k#} form#) 
                                                   (str "\t" k# " : " v# sep#))) 
                                               ~bindings)))))))))
</code></pre>

So that. now it's just:
<pre><code>(let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= x y)))
java.lang.AssertionError: Assert failed: (= x y)
 x : 1
 y : 2
 (NO_SOURCE_FILE:0)

:f

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

fogus said: Hmmm, but that fails entirely for: (let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= [x y] [a c]))). So maybe it's better just to print all of the locals unless you really want to get complicated.
:f

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

jawolfe said: See also some comments in:

http://groups.google.com/group/clojure-dev/browse_frm/thread/68d49cd7eb4a4899/9afc6be4d3f8ae27?lnk=gst&q=assert#9afc6be4d3f8ae27

Plus one more suggestion to add to the mix: in addition to / instead of printing the locals, how about saving them somewhere. For example, the var assert-bindings could be bound to the map of locals. This way you don't run afoul of infinite/very large sequences, and allow the user to do more detailed interrogation of the bad values (especially useful when some of the locals print opaquely).

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

stuart.sierra said: Another approach, which I wil willingly donate:
http://github.com/stuartsierra/lazytest/blob/master/src/main/clojure/lazytest/expect.clj

Comment by Jeff Weiss [ 15/Dec/10 1:33 PM ]

There's one more tweak to fogus's last comment, which I'm actually using. You need to flatten the quoted form before you can use 'some' to check whether the local was used in the form:

(defmacro assert
  "Evaluates expr and throws an exception if it does not evaluate to logical true."
  {:added "1.0"}
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                          (map (fn [[k# v#]] 
                                                 (when (some #{k#} (flatten form#)) 
                                                   (str "\t" k# " : " v# sep#))) 
                                               ~bindings)))))))))
Comment by Stuart Halloway [ 04/Jan/11 8:31 PM ]

I am holding off on this until we have more solidity around http://dev.clojure.org/display/design/Error+Handling. (Considering, for instance, having all exceptions thrown from Clojure provide access to locals.)

When my pipe dream fades I will come back and screen this before the next release.

Comment by Stuart Halloway [ 28/Jan/11 1:14 PM ]

Why try to guess what someone wants to do with the locals (or any other context, for that matter) when you can specify a callback (see below). This would have been useful last week when I had an assertion that failed only on the CI box, where no debugger is available.

Rich, at the risk of beating a dead horse, I still think this is a good idea. Debuggers are not always available, and this is an example of where a Lisp is intrinsically capable of providing better information than can be had in other environments. If you want a patch for the code below please mark waiting on me, otherwise please decline this ticket so I stop looking at it.

(def ^:dynamic *assert-handler* nil)

(defn ^{:private true} local-bindings
  "Produces a map of the names of local bindings to their values."
  [env]
  (let [symbols (map key env)]
    (zipmap (map (fn [sym] `(quote ~sym)) symbols) symbols)))

(defmacro assert
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (if *assert-handler*
             (*assert-handler* form# ~bindings)
             (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                            (map (fn [[k# v#]] 
                                                   (when (some #{k#} (flatten form#)) 
                                                     (str "\t" k# " : " v# sep#))) 
                                                 ~bindings))))))))))
Comment by Jeff Weiss [ 27/May/11 8:16 AM ]

A slight improvement I made in my own version of this code: flatten does not affect set literals. So if you do (assert (some #{x} [a b c d])) the value of x will not be printed. Here's a modified flatten that does the job:

(defn symbols [sexp]
  "Returns just the symbols from the expression, including those
   inside literals (sets, maps, lists, vectors)."
  (distinct (filter symbol? (tree-seq coll? seq sexp))))
Comment by Andy Fingerhut [ 18/Nov/12 1:06 AM ]

Attaching git format patch clj-415-assert-prints-locals-v1.txt of Stuart Halloway's version of this idea. I'm not advocating it over the other variations, just getting a file attached to the JIRA ticket.

Comment by Michael Blume [ 05/Jul/15 7:53 PM ]

Previous patch was incompatible with CLJ-1005, which moves zipmap later in clojure.core. Rewrote to use into.

Comment by Michael Blume [ 06/Jul/15 3:30 PM ]

Both patches are somehow incompatible with CLJ-1224. When building my compojure-api project I get

Exception in thread "main" java.lang.UnsupportedOperationException: Can't type hint a primitive local, compiling:(schema/core.clj:680:27)
	at clojure.lang.Compiler.analyze(Compiler.java:6569)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$MapExpr.parse(Compiler.java:3050)
	at clojure.lang.Compiler.analyze(Compiler.java:6558)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3791)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6751)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$NewExpr$Parser.parse(Compiler.java:2614)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$ThrowExpr$Parser.parse(Compiler.java:2409)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5889)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2785)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5889)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6205)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2777)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2785)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2785)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5322)
	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3925)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6747)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6205)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2777)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6205)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$NewInstanceMethod.parse(Compiler.java:8165)
	at clojure.lang.Compiler$NewInstanceExpr.build(Compiler.java:7672)
	at clojure.lang.Compiler$NewInstanceExpr$DeftypeParser.parse(Compiler.java:7549)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5889)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6205)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5322)
	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3925)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6747)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.eval(Compiler.java:6805)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at plumbing.fnk.schema$eval12507$loading__5352__auto____12508.invoke(schema.clj:1)
	at plumbing.fnk.schema$eval12507.invoke(schema.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at plumbing.core$eval12244$loading__5352__auto____12245.invoke(core.clj:1)
	at plumbing.core$eval12244.invoke(core.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:930)
	at compojure.api.meta$eval11960$loading__5352__auto____11961.invoke(meta.clj:1)
	at compojure.api.meta$eval11960.invoke(meta.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:703)
	at compojure.api.core$eval11954$loading__5352__auto____11955.invoke(core.clj:1)
	at compojure.api.core$eval11954.invoke(core.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at compojure.api.sweet$eval11948$loading__5352__auto____11949.invoke(sweet.clj:1)
	at compojure.api.sweet$eval11948.invoke(sweet.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:703)
	at com.climate.scouting.resources$eval10202$loading__5352__auto____10203.invoke(resources.clj:1)
	at com.climate.scouting.resources$eval10202.invoke(resources.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5761)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:703)
	at com.climate.scouting.service$eval8256$loading__5352__auto____8257.invoke(service.clj:1)
	at com.climate.scouting.service$eval8256.invoke(service.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at com.climate.scouting.dev_server$eval8250$loading__5352__auto____8251.invoke(dev_server.clj:1)
	at com.climate.scouting.dev_server$eval8250.invoke(dev_server.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at user$eval58$fn__69.invoke(form-init9085313321330645488.clj:1)
	at user$eval58.invoke(form-init9085313321330645488.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6798)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.Compiler.loadFile(Compiler.java:7191)
	at clojure.main$load_script.invoke(main.clj:275)
	at clojure.main$init_opt.invoke(main.clj:280)
	at clojure.main$initialize.invoke(main.clj:308)
	at clojure.main$null_opt.invoke(main.clj:343)
	at clojure.main$main.doInvoke(main.clj:421)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.UnsupportedOperationException: Can't type hint a primitive local
	at clojure.lang.Compiler$LocalBindingExpr.<init>(Compiler.java:5792)
	at clojure.lang.Compiler.analyzeSymbol(Compiler.java:6929)
	at clojure.lang.Compiler.analyze(Compiler.java:6532)
	... 299 more
Failed.
Comment by Andy Fingerhut [ 06/Jul/15 4:02 PM ]

Michael, are you finding these incompatibilities between patches because you want to run a modified version of Clojure with all of these patches? Understood, if so.

If you are looking for pairs of patches that are incompatible with each other, I'd recommend a different hobby They get applied at the rate of about 9 per month, on average, so there should be plenty of time to resolve inconsistencies between them later.





[CLJ-405] better error messages for bad defrecord calls Created: 20/Jul/10  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord, errormsgs


 Description   

defrecord could tell you if, e.g., you didn't specify an interface before leaping into method bodies. See http://groups.google.com/group/clojure/browse_thread/thread/f52f90954edd8b09



 Comments   
Comment by Assembla Importer [ 24/Aug/10 12:28 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 12:28 AM ]

stu said: This could be fixed with an assert-valid-defrecord call in core_deftype, similar to assert-valid-fdecl in core.clj. Such a function would also be a place to hang other defrecord error messages.





[CLJ-233] better error reporting of nonexistent var Created: 31/Dec/09  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs


 Description   

simple improvement to error message when referencing a var that doesn't exist.



 Comments   
Comment by Assembla Importer [ 29/Sep/10 5:29 AM ]

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

Comment by Assembla Importer [ 29/Sep/10 5:29 AM ]

chouser@n01se.net said: Stuart, I don't see a patch attached.





[CLJ-148] Poor reporting of symbol conflicts when using (ns) Created: 10/Jul/09  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs


 Description   

I have a module that includes pprint and my own utils.

When com.howard.lewisship.cascade.dom/write was changed from private to public I get the following error:

java.lang.IllegalStateException: write already refers to: #'clojure.contrib.pprint/write in namespace: com.howardlewisship.cascade.test-views (test_views.clj:0)

(ns com.howardlewisship.cascade.test-views ; line 15
(:use
(clojure.contrib test-is pprint duck-streams)
(app1 views fragments)
(com.howardlewisship.cascade config dom view-manager)
com.howardlewisship.cascade.internal.utils))

That line number is wrong but better yet, identifying the true conflict (com.howard.lewisship.cascade.dom/write) would be even more important.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 3:54 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 3:54 AM ]

scgilardi said: It's saying that the symbol com.howardlewisship.cascade.test-views/write already resolves to #���clojure.contrib.pprint/write, so you can't def a new write in com.howardlewisship.cascade.test-views.

What do you propose for an alternate wording of the error message here?

Comment by Jeff Rose [ 12/May/11 9:49 AM ]

I think the issue is that only one side of the conflict is reported in the error, so if you get this kind of error in the middle of a large project it can be hard to figure out which namespace is conflicting. Take a toy example:

user=> (ns foo)
foo=> (def foobar 42)
foo=> (ns bar)
bar=> (def foobar 0)
bar=> (ns problem)
problem=> (refer 'foo)
problem=> (refer 'bar)
java.lang.IllegalStateException: foobar already refers to: #'foo/foobar in namespace: problem (NO_SOURCE_FILE:0)

In this case it would be best if the error said something like:

"Conflict referring to #'bar/foobar in #<Namespace problem> because foobar already refers to: #'foo/foobar."

This way the error message clearly identifies the location of the conflict, and the locations of the two conflicting vars.

Hopefully this helps clarify. I think I see where to fix it in warnOrFailOnReplace on line 88 of src/jvm/clojure/lang/Namespace.java, and this reminds me I need to send in a CA so I can pitch in next time...

Comment by Aaron Bedra [ 28/Jun/11 6:42 PM ]

It looks like the true conflict is in test-views, not in dom. A small example of the line number breakage showing the problem on master (1.3) would be very helpful.





[CLJ-77] GC Issue 74: Clojure compiler emits too-large classfiles (results in ClassFormatError) Created: 17/Jun/09  Updated: 23/Feb/15

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

Type: Defect Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs


 Description   
Reported by cemer...@snowtide.com, Feb 10, 2009

The jvm has certain implementation limits around the maximum size of
classfiles, literal strings, method length, etc; however, in certain
circumstances, the Clojure compiler can currently emit classfiles that
violate some of those limitations, causing an error later when the
classfile is loaded.

While test coverage would necessarily detect this sort of problem on a
project-by-project basis when one's tests attempted to load a project's
classfiles, it seems like Clojure should do the following to ensure failure
as quickly as possible:

- throw an exception immediately if, while compiling a lib, it is detected
that the resulting classfile(s) would violate any classfile implementation
limits.  Ideally, the exception's message would detail what file and on
which line number the offending form is (e.g. if a method's bytecode would
be too long).  I can imagine that doing this may not be straightforward; a
reasonable stop-gap would be for the compiler to immediately attempt to
load the generated classfile in order to ensure up-front failure.

- emit a warning if any clojure form is read that would, upon being
compiled, require violating any of the classfile implementation limits; I
suspect that *most* people looking to generate classfiles would be doing so
in a "build" environment (rather than loading some code, tinkering, and
then using clojure.core/compile), but for those that aren't, I can imagine
there being a good deal of frustration around seeing that loading and using
some code successfully would eventually produce unusable classfiles.

I've appended a sample stack trace emitted by java when it attempted to
load a too-long method implementation (which was produced by embedding a
large list literal in a compiled lib).

Exception in thread "main" java.lang.ClassFormatError: Invalid method  
Code length 105496 in class file com/foo/MyClass__init
         at java.lang.ClassLoader.defineClass1(Native Method)
         at java.lang.ClassLoader.defineClass(ClassLoader.java:675)
         at  
java.security.SecureClassLoader.defineClass(SecureClassLoader.java:124)
         at java.net.URLClassLoader.defineClass(URLClassLoader.java:260)
         at java.net.URLClassLoader.access$000(URLClassLoader.java:56)
         at java.net.URLClassLoader$1.run(URLClassLoader.java:195)
         at java.security.AccessController.doPrivileged(Native Method)
         at java.net.URLClassLoader.findClass(URLClassLoader.java:188)
         at java.lang.ClassLoader.loadClass(ClassLoader.java:316)
         at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:
288)
         at java.lang.ClassLoader.loadClass(ClassLoader.java:251)
         at java.lang.ClassLoader.loadClassInternal(ClassLoader.java:
374)
         at java.lang.Class.forName0(Native Method)
         at java.lang.Class.forName(Class.java:247)
         at clojure.lang.RT.loadClassForName(RT.java:1512)
         at clojure.lang.RT.load(RT.java:394)
         at clojure.lang.RT.load(RT.java:374)
         at clojure.core$load__4911$fn__4913.invoke(core.clj:3623)
         at clojure.core$load__4911.doInvoke(core.clj:3622)
         at clojure.lang.RestFn.invoke(RestFn.java:413)
         at clojure.core$load_one__4863.invoke(core.clj:3467)
         at clojure.core$compile__4918$fn__4920.invoke(core.clj:3633)
         at clojure.core$compile__4918.invoke(core.clj:3632)
         at clojure.lang.Var.invoke(Var.java:336)
         at clojure.lang.Compile.main(Compile.java:56)


 Comments   
Comment by Assembla Importer [ 24/Aug/10 6:45 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 6:45 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)





[CLJ-5] Unintuitive error response in clojure 1.0 Created: 17/Jun/09  Updated: 18/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

Attachments: File clj-5-destructure-error.diff     Text File CLJ-5.patch    
Patch: Code and Test
Approval: Vetted

 Description   

The following broken code:

(let [[x y] {}] x)

provides the following stack trace:

Exception in thread "main" java.lang.UnsupportedOperationException: nth not supported on this type: PersistentArrayMap (test.clj:0)
at clojure.lang.Compiler.eval(Compiler.java:4543)
at clojure.lang.Compiler.load(Compiler.java:4857)
at clojure.lang.Compiler.loadFile(Compiler.java:4824)
at clojure.main$load_script__5833.invoke(main.clj:206)
at clojure.main$script_opt__5864.invoke(main.clj:258)
at clojure.main$main__5888.doInvoke(main.clj:333)
at clojure.lang.RestFn.invoke(RestFn.java:413)
at clojure.lang.Var.invoke(Var.java:346)
at clojure.lang.AFn.applyToHelper(AFn.java:173)
at clojure.lang.Var.applyTo(Var.java:463)
at clojure.main.main(main.java:39)
Caused by: java.lang.UnsupportedOperationException: nth not supported on this type: PersistentArrayMap
at clojure.lang.RT.nth(RT.java:800)
at clojure.core$nth__3578.invoke(core.clj:873)
at user$eval__1.invoke(test.clj:1)
at clojure.lang.Compiler.eval(Compiler.java:4532)
... 10 more

The message "nth not supported on this type" while correct doesn't make the cause of the error very clear. Better error messages when destructuring would be very helpful.



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

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

Comment by Eugene Koontz [ 11/Nov/11 7:36 PM ]

Please see the attached patch which produces a (hopefully more clear) error message as shown below (given the broken code shown in the original bug report):

Clojure 1.4.0-master-SNAPSHOT
user=> (let [x 42 y 43] (+ x y))
85
user=> (let [[x y] {}] x)
UnsupportedOperationException left side of binding must be a symbol (found a PersistentVector instead).  clojure.lang.Compiler.checkLet (Compiler.java:6545)
user=>

In addition, this patch checks the argument of (let) as shown below:

user=> (let 42)
UnsupportedOperationException argument to (let)  must be a vector (found a Long instead).  clojure.lang.Compiler.checkLet (Compiler.java:6553)
Comment by Eugene Koontz [ 11/Nov/11 7:38 PM ]

Patch produced by doing git diff against commit ba930d95fc (master branch).

Comment by Eugene Koontz [ 13/Nov/11 11:24 PM ]

Sorry, this patch is wrong: it assumes that the left side of the binding is wrong - the [x y] in :

(let [[x y] {}] x)

because [x y] is a vector, when in fact, the left side is fine (per http://clojure.org/special_forms#let : "Clojure supports abstract structural binding, often called destructuring, in let binding lists".)

So it's the right side (the {}) that needs to be checked and flagged as erroneous, not the [x y].

Comment by Carin Meier [ 30/Nov/11 12:15 PM ]

Add patch better-error-for-let-vector-map-binding

This produces the following:

(let [[x y] {}] x)
Exception map binding to vector is not supported

There are other cases that are not handled by this though — like binding vector to a set

user=> (let [[x y] #{}] x)
UnsupportedOperationException nth not supported on this type: PersistentHashSet

Wondering if it might be better to try convert the map to a seq to support? Although this might be another issue.

Thoughts?

Comment by Aaron Bedra [ 30/Nov/11 7:12 PM ]

This seems too specific. Is this issue indicative of a larger problem that should be addressed? Even if this is the only case where bindings produce poor error messages, all the cases described above should be addressed in the patch.

Comment by Carin Meier [ 16/Dec/11 7:47 AM ]

Unfortunately, realized that this still does not cover the nested destructuring cases. Coming to the conclusion, that my approach above is not going to work for this.

Comment by Carin Meier [ 28/Apr/12 10:46 PM ]

File: clj-5-destructure-error.diff

Added support for nested destructuring errors

let [[[x1 y1][x2 y2]] [[1 2] {}]]
;=> UnsupportedOperationException let cannot destructure class clojure.lang.PersistentArrayMap.
Comment by Kevin Downey [ 18/Apr/14 1:45 AM ]

I am not wild about that error message, let can destructure a map fine.

If there error message were to change, I would prefer to get something like "sequential destructing not supported on maps".

I actually like the "nth not supported" error message, because it is exactly the problem, nth, used by sequential destructuring, doesn't work on maps.

it conveys exactly what the problem is if you know how destructing works and what nth means, where as "UnsupportedOperationException let cannot destructure class clojure.lang.PersistentArrayMap" seems misleading when you are in the know





[ASYNC-98] Less hostile message for #'go stopping at (fn [] ) boundaries Created: 15/Oct/14  Updated: 15/Oct/14

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

Type: Enhancement Priority: Minor
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs


 Description   

ASYNC-93 has an example of a not nice message when #'go attempts to shred through a (fn []) form. Maybe we can improve this, but then again creating a (fn []) in a block should be permitted, though the inner contents of it will not be transformed.






[ASYNC-38] keep</> instead of map</> Created: 18/Nov/13  Updated: 02/Sep/14  Resolved: 02/Sep/14

Status: Closed
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: documentation, enhancement, errormsgs


 Description   

One problem with using map< is when the supplied function returns nil. In such case (using the latest implementation from core.async [org.clojure/core.async "0.1.256.0-1bf8cf-alpha"]) one can take nil from a channel created with map<. This is otherwise only possible with closed channels. Putting nil on a channel normally throws an IllegalArgumentException.

With the current implementation of map< it is not possible to determine whether the source-channel was closed or the supplied function returned nil.

Notice that putting onto a channel created with map> throws an IllegalArgumentException when the supplied function returns nil as if you would put nil onto a channel.

My proposal is to add keep</> (where nil values returned from the supplied function are ignored and nothing is put) to the core library or to implement map</> as keep</> since having a real map</> instead of keep</> hardly makes sense when nil is not permitted anyways.



 Comments   
Comment by Leon Grapenthin [ 24/Apr/14 3:44 AM ]

It is still an issue in "0.1.278.0-76b25b-alpha" that you can only use impl.protocols/closed? to consistently determine whether a channel created with map</> was closed - if nil is one of your predicates return values.

Of course, you could use a predicate that never returns nil. But what should be the benefit of map</> being able to magically pass nil while everything else isn't?

Comment by Alex Miller [ 02/Sep/14 9:43 AM ]

All of the transformation functions (like map<) are deprecated and will go away to be replaced with applying transducers to channels.





[ASYNC-27] Compilation errors inside go block always reported as first line of block Created: 08/Oct/13  Updated: 09/May/14

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

Type: Defect Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: errormsgs


 Description   

I've noticed that when there are any errors inside a go block, the line number of the error is always the line containing the go symbol.

I suspect that some meta data on the forms that are converted into a state machine is being lost in the process.

This is quite annoying and quite leaky (in the abstraction sense). It makes it that much harder to track down the source of errors.



 Comments   
Comment by Timothy Baldridge [ 22/Nov/13 2:49 PM ]

Working on this, may be a few weeks out yet.

Comment by Hugo Duncan [ 11/Feb/14 5:18 PM ]

This effects both compilation errors and line numbers in stack trace frames.

Comment by Timothy Baldridge [ 09/May/14 11:26 AM ]

Completed in CLJ, once we get a tools.analyzer.cljs I'll add this to CLJS as well.





Generated at Fri Aug 28 12:26:23 CDT 2015 using JIRA 4.4#649-r158309.