Skip to content
This repository has been archived by the owner on Jan 28, 2019. It is now read-only.

Commit

Permalink
Add meaningful messages to common RT errors
Browse files Browse the repository at this point in the history 
This patch wraps many of the collections functions in clojure.lang.RT
with try/catch blocks designed to provide clearer error reporting when
the user provides them with inappropriate arguments.

Previously raw ClassCastExceptions from cast sites could be propagated
to the user. While precise, these errors are unclear because they don't
state what expectation was violated, Eg. object on which a cast needed
to be sequable or a map or such. This leaves users, especially new
users, exposed to the raw implementation of Clojure in terms of Java
interfaces and not stated Clojure abstractions such as maps and
seqs.

Ex.

```clojure
(assoc (Object.) :foo :bar)
;; IllegalArgumentException assoc needs an associative, got java.lang.Object  clojure.lang.RT.assoc (RT.java:820)
```

This does technically make this patch a breaking change, since it
changes the exception thrown by many type missmatches from a
ClassCastException to a more user meaningful IllegalArgumentException.
  • Loading branch information
@arrdem
arrdem committed Nov 20, 2015
1 parent c6f89a4 commit e1e7c08
Show file tree
Hide file tree
Showing 2 changed files with 107 additions and 59 deletions.
156 changes: 102 additions & 54 deletions src/jvm/clojure/lang/RT.java
View file
Expand Up @@ -645,7 +645,7 @@ else if (o instanceof Map.Entry)
else if(o.getClass().isArray())
return Array.getLength(o);

throw new UnsupportedOperationException("count not supported on this type: " + o.getClass().getSimpleName());
throw new UnsupportedOperationException("count not supported on this type: " + o.getClass().getName());
}

static public IPersistentCollection conj(IPersistentCollection coll, Object x){
Expand All @@ -655,52 +655,80 @@ static public IPersistentCollection conj(IPersistentCollection coll, Object x){
}

static public ISeq cons(Object x, Object coll){
//ISeq y = seq(coll);
if(coll == null)
return new PersistentList(x);
else if(coll instanceof ISeq)
return new Cons(x, (ISeq) coll);
else
return new Cons(x, seq(coll));
try {
if(coll == null) {
return new PersistentList(x);
} else if(coll instanceof ISeq) {
return new Cons(x, (ISeq) coll);
} else {
return new Cons(x, seq(coll));
}
} catch(IllegalArgumentException e) {
throw new IllegalArgumentException("cons needs a sequable, got " + coll.getClass().getName());
}
}

static public Object first(Object x){
if(x instanceof ISeq)
return ((ISeq) x).first();
ISeq seq = seq(x);
if(seq == null)
return null;
return seq.first();
try {
if(x instanceof ISeq)
return ((ISeq) x).first();
ISeq seq = seq(x);
if(seq == null)
return null;
return seq.first();
} catch(IllegalArgumentException e) {
throw new IllegalArgumentException("first needs a sequable, got " + x.getClass().getName());
}
}

static public Object second(Object x){
return first(next(x));
try {
return first(next(x));
} catch(IllegalArgumentException e) {
throw new IllegalArgumentException("second needs a sequable, got " + x.getClass().getName());
}
}

static public Object third(Object x){
return first(next(next(x)));
try {
return first(next(next(x)));
} catch(IllegalArgumentException e) {
throw new IllegalArgumentException("third needs a sequable, got " + x.getClass().getName());
}
}

static public Object fourth(Object x){
return first(next(next(next(x))));
try {
return first(next(next(next(x))));
} catch(IllegalArgumentException e) {
throw new IllegalArgumentException("fourth needs a sequable, got " + x.getClass().getName());
}
}

static public ISeq next(Object x){
if(x instanceof ISeq)
return ((ISeq) x).next();
ISeq seq = seq(x);
if(seq == null)
return null;
return seq.next();
try {
if(x instanceof ISeq)
return ((ISeq) x).next();
ISeq seq = seq(x);
if(seq == null)
return null;
return seq.next();
} catch(IllegalArgumentException e) {
throw new IllegalArgumentException("next needs a sequable, got " + x.getClass().getName());
}
}

static public ISeq more(Object x){
if(x instanceof ISeq)
return ((ISeq) x).more();
ISeq seq = seq(x);
if(seq == null)
return PersistentList.EMPTY;
return seq.more();
try {
if(x instanceof ISeq)
return ((ISeq) x).more();
ISeq seq = seq(x);
if(seq == null)
return PersistentList.EMPTY;
return seq.more();
} catch(IllegalArgumentException e) {
throw new IllegalArgumentException("more needs a sequable, got " + x.getClass().getName());
}
}

//static public Seqable more(Object x){
Expand All @@ -721,15 +749,23 @@ static public ISeq more(Object x){
//}

static public Object peek(Object x){
if(x == null)
return null;
return ((IPersistentStack) x).peek();
try {
if(x == null)
return null;
return ((IPersistentStack) x).peek();
} catch(ClassCastException e) {
throw new IllegalArgumentException("peek needs a queue, got " + x.getClass().getName());
}
}

static public Object pop(Object x){
if(x == null)
return null;
return ((IPersistentStack) x).pop();
try {
if(x == null)
return null;
return ((IPersistentStack) x).pop();
} catch(ClassCastException e) {
throw new IllegalArgumentException("pop needs a queue, got " + x.getClass().getName());
}
}

static public Object get(Object coll, Object key){
Expand Down Expand Up @@ -789,9 +825,13 @@ else if(key instanceof Number && (coll instanceof String || coll.getClass().isAr
}

static public Associative assoc(Object coll, Object key, Object val){
if(coll == null)
return new PersistentArrayMap(new Object[]{key, val});
return ((Associative) coll).assoc(key, val);
try {
if(coll == null)
return new PersistentArrayMap(new Object[]{key, val});
return ((Associative) coll).assoc(key, val);
} catch(ClassCastException e) {
throw new IllegalArgumentException("assoc needs an associative, got " + coll.getClass().getName());
}
}

static public Object contains(Object coll, Object key){
Expand All @@ -817,16 +857,20 @@ else if(key instanceof Number && (coll instanceof String || coll.getClass().isAr
}

static public Object find(Object coll, Object key){
if(coll == null)
return null;
else if(coll instanceof Associative)
return ((Associative) coll).entryAt(key);
else {
Map m = (Map) coll;
if(m.containsKey(key))
return Tuple.create(key, m.get(key));
return null;
}
try {
if(coll == null)
return null;
else if(coll instanceof Associative)
return ((Associative) coll).entryAt(key);
else {
Map m = (Map) coll;
if(m.containsKey(key))
return Tuple.create(key, m.get(key));
return null;
}
} catch(ClassCastException e) {
throw new IllegalArgumentException("find needs a map or associative, got a " + coll.getClass().getName());
}
}

//takes a seq of key,val,key,val
Expand All @@ -845,9 +889,13 @@ static public ISeq findKey(Keyword key, ISeq keyvals) {
}

static public Object dissoc(Object coll, Object key) {
if(coll == null)
return null;
return ((IPersistentMap) coll).without(key);
try {
if(coll == null)
return null;
return ((IPersistentMap) coll).without(key);
} catch(ClassCastException e) {
throw new IllegalArgumentException("dissoc needs an IPersistentMap, got " + coll.getClass().getName());
}
}

static public Object nth(Object coll, int n){
Expand Down Expand Up @@ -893,9 +941,9 @@ else if(coll instanceof Sequential) {

static public Object nth(Object coll, int n, Object notFound){
if(coll instanceof Indexed) {
Indexed v = (Indexed) coll;
return v.nth(n, notFound);
}
Indexed v = (Indexed) coll;
return v.nth(n, notFound);
}
return nthFrom(coll, n, notFound);
}

Expand Down
10 changes: 5 additions & 5 deletions test/clojure/test_clojure/data_structures.clj
View file
Expand Up @@ -345,8 +345,8 @@

(deftest test-peek
; doesn't work for sets and maps
(is (thrown? ClassCastException (peek #{1})))
(is (thrown? ClassCastException (peek {:a 1})))
(is (thrown? IllegalArgumentException (peek #{1})))
(is (thrown? IllegalArgumentException (peek {:a 1})))

(are [x y] (= x y)
(peek nil) nil
Expand Down Expand Up @@ -378,8 +378,8 @@

(deftest test-pop
; doesn't work for sets and maps
(is (thrown? ClassCastException (pop #{1})))
(is (thrown? ClassCastException (pop #{:a 1})))
(is (thrown? IllegalArgumentException (pop #{1})))
(is (thrown? IllegalArgumentException (pop #{:a 1})))

; collection cannot be empty
(is (thrown? IllegalStateException (pop ())))
Expand Down Expand Up @@ -1287,4 +1287,4 @@
(defspec seq-and-iter-match-for-structs
identity
[^{:tag clojure.test-clojure.data-structures/gen-struct} s]
(seq-iter-match s s))
(seq-iter-match s s))

0 comments on commit e1e7c08

Please sign in to comment.