Skip to content

Commit 1c8eb16

Browse files
frenchy64Stuart Sierra
frenchy64
authored and
Stuart Sierra
committed
Better error messages for syntax errors w/ defn and fn [CLJ-157]
Breaking changes: (fn) (fn a) Both of these now throw exceptions instead of silently returning an undefined (according to the docstring of fn) function. See tests in clojure.test-clojure.def and clojure.test-clojure.fn for specific new error messages. While defn uses fn, many changes had to be repeated in defn because defn analyses the arglists for metadata before passing to fn. Signed-off-by: Stuart Sierra <mail@stuartsierra.com>
1 parent d4170e6 commit 1c8eb16

4 files changed

Lines changed: 141 additions & 6 deletions

File tree

‎src/clj/clojure/core.clj‎

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,10 @@
270270
[name doc-string? attr-map? ([params*] prepost-map? body)+ attr-map?])
271271
:added "1.0"}
272272
defn (fn defn [&form &env name & fdecl]
273+
;; Note: Cannot delegate this check to def because of the call to (with-meta name ..)
274+
(if (instance? clojure.lang.Symbol name)
275+
nil
276+
(throw (IllegalArgumentException. "First argument to defn must be a symbol")))
273277
(let [m (if (string? (first fdecl))
274278
{:doc (first fdecl)}
275279
{})
@@ -4031,9 +4035,31 @@
40314035
[& sigs]
40324036
(let [name (if (symbol? (first sigs)) (first sigs) nil)
40334037
sigs (if name (next sigs) sigs)
4034-
sigs (if (vector? (first sigs)) (list sigs) sigs)
4038+
sigs (if (vector? (first sigs))
4039+
(list sigs)
4040+
(if (seq? (first sigs))
4041+
sigs
4042+
;; Assume single arity syntax
4043+
(throw (IllegalArgumentException.
4044+
(if (seq sigs)
4045+
(str "Parameter declaration "
4046+
(first sigs)
4047+
" should be a vector")
4048+
(str "Parameter declaration missing"))))))
40354049
psig (fn* [sig]
4050+
;; Ensure correct type before destructuring sig
4051+
(when (not (seq? sig))
4052+
(throw (IllegalArgumentException.
4053+
(str "Invalid signature " sig
4054+
" should be a list"))))
40364055
(let [[params & body] sig
4056+
_ (when (not (vector? params))
4057+
(throw (IllegalArgumentException.
4058+
(if (seq? (first sigs))
4059+
(str "Parameter declaration " params
4060+
" should be a vector")
4061+
(str "Invalid signature " sig
4062+
" should be a list")))))
40374063
conds (when (and (next body) (map? (first body)))
40384064
(first body))
40394065
body (if conds (next body) body)
@@ -6622,8 +6648,24 @@
66226648
(defn- ^{:dynamic true} assert-valid-fdecl
66236649
"A good fdecl looks like (([a] ...) ([a b] ...)) near the end of defn."
66246650
[fdecl]
6625-
(if-let [bad-args (seq (remove #(vector? %) (map first fdecl)))]
6626-
(throw (IllegalArgumentException. (str "Parameter declaration " (first bad-args) " should be a vector")))))
6651+
(when (empty? fdecl) (throw (IllegalArgumentException.
6652+
"Parameter declaration missing")))
6653+
(let [argdecls (map
6654+
#(if (seq? %)
6655+
(first %)
6656+
(throw (IllegalArgumentException.
6657+
(if (seq? (first fdecl))
6658+
(str "Invalid signature "
6659+
%
6660+
" should be a list")
6661+
(str "Parameter declaration "
6662+
%
6663+
" should be a vector")))))
6664+
fdecl)
6665+
bad-args (seq (remove #(vector? %) argdecls))]
6666+
(when bad-args
6667+
(throw (IllegalArgumentException. (str "Parameter declaration " (first bad-args)
6668+
" should be a vector"))))))
66276669

66286670
(defn with-redefs-fn
66296671
"Temporarily redefines Vars during a call to func. Each val of

‎src/script/run_tests.clj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ clojure.test-clojure.data-structures
1515
clojure.test-clojure.def
1616
clojure.test-clojure.errors
1717
clojure.test-clojure.evaluation
18+
clojure.test-clojure.fn
1819
clojure.test-clojure.for
1920
clojure.test-clojure.genclass.examples
2021
clojure.test-clojure.genclass

‎test/clojure/test_clojure/def.clj‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,46 @@
1111
clojure.test-clojure.protocols))
1212

1313
(deftest defn-error-messages
14-
(testing "bad arglist forms"
15-
(is (fails-with-cause? IllegalArgumentException '#"Parameter declaration arg1 should be a vector"
16-
(eval-in-temp-ns (defn foo (arg1 arg2)))))))
14+
(testing "multiarity syntax invalid parameter declaration"
15+
(is (fails-with-cause?
16+
IllegalArgumentException
17+
#"Parameter declaration arg1 should be a vector"
18+
(eval-in-temp-ns (defn foo (arg1 arg2))))))
19+
20+
(testing "multiarity syntax invalid signature"
21+
(is (fails-with-cause?
22+
IllegalArgumentException
23+
#"Invalid signature \[a b\] should be a list"
24+
(eval-in-temp-ns (defn foo
25+
([a] 1)
26+
[a b])))))
27+
28+
(testing "assume single arity syntax"
29+
(is (fails-with-cause?
30+
IllegalArgumentException
31+
#"Parameter declaration a should be a vector"
32+
(eval-in-temp-ns (defn foo a)))))
33+
34+
(testing "bad name"
35+
(is (fails-with-cause?
36+
IllegalArgumentException
37+
#"First argument to defn must be a symbol"
38+
(eval-in-temp-ns (defn "bad docstring" testname [arg1 arg2])))))
39+
40+
(testing "missing parameter/signature"
41+
(is (fails-with-cause?
42+
IllegalArgumentException
43+
#"Parameter declaration missing"
44+
(eval-in-temp-ns (defn testname)))))
45+
46+
(testing "allow trailing map"
47+
(is (eval-in-temp-ns (defn a "asdf" ([a] 1) {:a :b}))))
48+
49+
(testing "don't allow interleaved map"
50+
(is (fails-with-cause?
51+
IllegalArgumentException
52+
#"Invalid signature \{:a :b\} should be a list"
53+
(eval-in-temp-ns (defn a "asdf" ([a] 1) {:a :b} ([] 1)))))))
1754

1855
(deftest dynamic-redefinition
1956
;; too many contextual things for this kind of caching to work...

‎test/clojure/test_clojure/fn.clj‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
; Copyright (c) Rich Hickey. All rights reserved.
2+
; The use and distribution terms for this software are covered by the
3+
; Eclipse Public License 1.0 (http://opensource.org/licenses/eclipse-1.0.php)
4+
; which can be found in the file epl-v10.html at the root of this distribution.
5+
; By using this software in any fashion, you are agreeing to be bound by
6+
; the terms of this license.
7+
; You must not remove this notice, or any other, from this software.
8+
9+
; Author: Ambrose Bonnaire-Sergeant
10+
11+
(ns clojure.test-clojure.fn
12+
(:use clojure.test))
13+
14+
(deftest fn-error-checking
15+
(testing "bad arglist"
16+
(is (fails-with-cause? java.lang.IllegalArgumentException
17+
#"Parameter declaration a should be a vector"
18+
(eval '(fn "a" a)))))
19+
20+
(testing "treat first param as args"
21+
(is (fails-with-cause? java.lang.IllegalArgumentException
22+
#"Parameter declaration a should be a vector"
23+
(eval '(fn "a" [])))))
24+
25+
(testing "looks like listy signature, but malformed declaration"
26+
(is (fails-with-cause? java.lang.IllegalArgumentException
27+
#"Parameter declaration 1 should be a vector"
28+
(eval '(fn (1))))))
29+
30+
(testing "checks each signature"
31+
(is (fails-with-cause? java.lang.IllegalArgumentException
32+
#"Parameter declaration a should be a vector"
33+
(eval '(fn
34+
([a] 1)
35+
("a" 2))))))
36+
37+
(testing "correct name but invalid args"
38+
(is (fails-with-cause? java.lang.IllegalArgumentException
39+
#"Parameter declaration a should be a vector"
40+
(eval '(fn a "a")))))
41+
42+
(testing "first sig looks multiarity, rest of sigs should be lists"
43+
(is (fails-with-cause? java.lang.IllegalArgumentException
44+
#"Invalid signature \[a b\] should be a list"
45+
(eval '(fn a
46+
([a] 1)
47+
[a b])))))
48+
49+
(testing "missing parameter declaration"
50+
(is (fails-with-cause? java.lang.IllegalArgumentException
51+
#"Parameter declaration missing"
52+
(eval '(fn a))))
53+
(is (fails-with-cause? java.lang.IllegalArgumentException
54+
#"Parameter declaration missing"
55+
(eval '(fn))))))

0 commit comments

Comments
 (0)