Fix multiple issues with destructuring (#1091)

Fixes #1078 
Fixes #1090

Author: chrisrink10

CHANGELOG.md

@@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
  * Fix an issue with `basilisp test` standard streams output that can lead to failures on MS-Windows (#1080)
+ * Fix an issue where destructuring a vector would throw an exception rather than returning `nil` for invalid key types (#1090)
+ * Fix an issue where destructuring default values would take precedence over falsey values in the source data structure (#1078)
 
 ### Removed
  * Removed support for Python 3.8 (#1083)

src/basilisp/core.lpy

@@ -5806,45 +5806,47 @@
                   (:children ddef))
 
         kw-binding (fn [arg]
-                     (let [kw-ns      (namespace arg)
-                           kw-name    (name arg)
-                           sym        (symbol kw-name)
-                           kw         (if kw-ns
-                                        (keyword kw-ns kw-name)
-                                        (keyword kw-name))
-                           or-binding (get ors sym)]
-                       (if or-binding
-                         [sym `(or (get ~fn-arg ~kw) ~or-binding)]
+                     (let [kw-ns   (namespace arg)
+                           kw-name (name arg)
+                           sym     (symbol kw-name)
+                           kw      (if kw-ns
+                                     (keyword kw-ns kw-name)
+                                     (keyword kw-name))]
+                       (if (contains? ors sym)
+                         [sym `(get ~fn-arg ~kw ~(get ors sym))]
                          [sym `(get ~fn-arg ~kw)])))
 
         map-binding (fn [f arg]
-                      (let [k          (f arg)
-                            or-binding (get ors arg)]
-                        (if or-binding
-                          [arg `(or (get ~fn-arg ~k) ~or-binding)]
+                      (let [k (f arg)]
+                        (if (contains? ors arg)
+                          [arg `(get ~fn-arg ~k ~(get ors arg))]
                           [arg `(get ~fn-arg ~k)])))
 
         sym-binding (fn [arg]
-                      (let [sym-name   (name arg)
-                            sym        (symbol sym-name)
-                            or-binding (get ors sym)]
-                        (if or-binding
-                          [sym `(or (get ~fn-arg (quote ~arg)) ~or-binding)]
+                      (let [sym-name (name arg)
+                            sym      (symbol sym-name)]
+                        (if (contains? ors sym)
+                          [sym `(get ~fn-arg (quote ~arg) ~(get ors sym))]
                           [sym `(get ~fn-arg (quote ~arg))])))
 
         named-binding (fn [arg]
-                        (let [binding    (get-in arg [:binding :name])
-                              key        (:key arg)
-                              or-binding (get ors binding)]
-                          (if or-binding
-                            [binding `(or (get ~fn-arg (quote ~key)) ~or-binding)]
+                        (let [binding (get-in arg [:binding :name])
+                              key     (:key arg)]
+                          (if (contains? ors binding)
+                            [binding `(get ~fn-arg (quote ~key) ~(get ors binding))]
                             [binding `(get ~fn-arg ~key)])))
 
         child-binding (fn [child]
                         (let [arg (get-in child [:binding :name])
                               k   (:key child)]
                           [arg `(get ~fn-arg ~k)]))]
     (concat
+     [fn-arg
+      `(if (seq? ~fn-arg)
+         (if (next ~fn-arg)
+           (apply hash-map ~fn-arg)
+           (first ~fn-arg))
+         ~fn-arg)]
      (mapcat kw-binding (:keys ddef))
      (mapcat (partial map-binding name) (:strs ddef))
      (mapcat sym-binding (:syms ddef))

src/basilisp/lang/runtime.py

@@ -1405,7 +1405,7 @@ def get(m, k, default=None):  # pylint: disable=unused-argument
 def _get_others(m, k, default=None):
     try:
         return m[k]
-    except (KeyError, IndexError):
+    except (KeyError, IndexError, TypeError):
         return default
 
 

src/basilisp/lang/vector.py

@@ -191,7 +191,7 @@ def entry(self, k: int) -> Optional[IMapEntry[int, T]]:
     def val_at(self, k: int, default: Optional[T] = None) -> Optional[T]:
         try:
             return self._inner[k]
-        except IndexError:
+        except (IndexError, TypeError):
             return default
 
     @staticmethod

tests/basilisp/runtime_test.py

@@ -392,7 +392,9 @@ def test_get():
     assert "nada" == runtime.get(vec.v(1, 2, 3).to_transient(), 3, "nada")
     assert "l" == runtime.get("hello world", 2)
     assert None is runtime.get("hello world", 50)
+    assert None is runtime.get("hello world", "key")
     assert "nada" == runtime.get("hello world", 50, "nada")
+    assert "nada" == runtime.get("hello world", "key", "nada")
     assert "l" == runtime.get(["h", "e", "l", "l", "o"], 2)
     assert None is runtime.get(["h", "e", "l", "l", "o"], 50)
     assert "nada" == runtime.get(["h", "e", "l", "l", "o"], 50, "nada")

tests/basilisp/test_core_macros.lpy

@@ -2,7 +2,7 @@
   (:import contextlib inspect os socket tempfile)
   (:require
    [basilisp.string :as str]
-   [basilisp.test :refer [deftest is testing]]))
+   [basilisp.test :refer [deftest is are testing]]))
 
 (deftest defn-test
   (testing "single arity defn"
@@ -739,6 +739,37 @@
              (g v))))))
 
 (deftest fn-associative-destructuring
+  (testing "ignore non-associative types"
+    (let [f (fn [{:keys [a]}]
+              a)]
+      (are [v] (nil? (f v))
+        nil
+        []
+        [:a 5]
+        ""
+        "abc"
+        5
+        5.5))
+
+    (testing "with defaults"
+      (let [f (fn [{:keys [a] :or {a :not-defined}}]
+                a)]
+        (are [v] (= :not-defined (f v))
+          nil
+          []
+          [:a 5]
+          ""
+          "abc"
+          5
+          5.5))))
+
+  (testing "falsey values not overridden by default values"
+    (let [f (fn [{:keys [a] :or {a :not-defined}}]
+              a)]
+      (are [v] (= v (f {:a v}))
+        nil
+        false)))
+
   (testing "destructuring simple keys"
     (let [m {:a "a" :b "b"}
 
@@ -754,11 +785,15 @@
 
     (let [m  {:a "a"}
           m1 {:a "a" :c "d"}
+          m2 '(:a "a" :c "d")
+          m3 (list m1)
 
           f (fn [{:keys [a c] :as m :or {c "c"}}]
               [a c m])]
       (is (= ["a" "c" m] (f m)))
-      (is (= ["a" "d" m1] (f m1)))))
+      (is (= ["a" "d" m1] (f m1)))
+      (is (= ["a" "d" (apply hash-map m2)] (f m2)))
+      (is (= ["a" "d" m1] (f m3)))))
 
   (testing "destructuring simple string keys"
     (let [m {:a 1 :b "b" "a" :a "b" :b}
@@ -1019,6 +1054,32 @@
       (is (= [[:a :b] [:c]] all)))))
 
 (deftest let-associative-destructuring
+  (testing "ignore non-associative types"
+    (are [v] (nil? (let [{:keys [a]} v] a))
+      nil
+      []
+      [:a 5]
+      ""
+      "abc"
+      5
+      5.5)
+
+    (testing "with defaults"
+      (are [v] (= :not-defined (let [{:keys [a] :or {a :not-defined}} v] a))
+        nil
+        []
+        [:a 5]
+        ""
+        "abc"
+        5
+        5.5)))
+
+  (testing "falsey values not overridden by default values"
+    (are [v] (= v (let [{:keys [a] :or {a :not-defined}} {:a v}]
+                    a))
+      nil
+      false))
+
   (testing "destructuring simple keys"
     (let [{:keys [a b]} {:a "a" :b "b"}]
       (is (= "a" a))
@@ -1032,7 +1093,17 @@
     (let [{:keys [a b] :as m :or {b "c"}} {:a "a"}]
       (is (= "a" a))
       (is (= "c" b))
-      (is (= {:a "a"} m))))
+      (is (= {:a "a"} m)))
+
+    (let [{:keys [a b] :as m :or {b "c"}} '(:a "a")]
+      (is (= "a" a))
+      (is (= "c" b))
+      (is (= {:a "a"} m)))
+
+    (let [{:keys [a b] :as m :or {b "c"}} '({:a "a" :b "b"})]
+      (is (= "a" a))
+      (is (= "b" b))
+      (is (= {:a "a" :b "b"} m))))
 
   (testing "destructuring simple string keys"
     (let [{:strs [a b]} {:a 1 :b 2 "a" :a "b" :b}]
@@ -1253,6 +1324,37 @@
                (g v)))))))
 
 (deftest letfn-associative-destructuring
+  (testing "ignore non-associative types"
+    (letfn [(f [{:keys [a]}]
+              a)]
+      (are [v] (nil? (f v))
+        nil
+        []
+        [:a 5]
+        ""
+        "abc"
+        5
+        5.5))
+
+    (testing "with defaults"
+      (letfn [(f [{:keys [a] :or {a :not-defined}}]
+                a)]
+        (are [v] (= :not-defined (f v))
+          nil
+          []
+          [:a 5]
+          ""
+          "abc"
+          5
+          5.5))))
+
+  (testing "falsey values not overridden by default values"
+    (letfn [(f [{:keys [a] :or {a :not-defined}}]
+              a)]
+      (are [v] (= v (f {:a v}))
+        nil
+        false)))
+
   (testing "destructuring simple keys"
     (let [m {:a "a" :b "b"}]
       (letfn [(f [{:keys [a]}]
@@ -1266,11 +1368,15 @@
         (is (= ["a" "b" m] (h m)))))
 
     (let [m  {:a "a"}
-          m1 {:a "a" :c "d"}]
+          m1 {:a "a" :c "d"}
+          m2 '(:a "a" :c "d")
+          m3 (list m1)]
       (letfn [(f [{:keys [a c] :as m :or {c "c"}}]
                 [a c m])]
         (is (= ["a" "c" m] (f m)))
-        (is (= ["a" "d" m1] (f m1))))))
+        (is (= ["a" "d" m1] (f m1)))
+        (is (= ["a" "d" (apply hash-map m2)] (f m2)))
+        (is (= ["a" "d" m1] (f m3))))))
 
   (testing "destructuring simple string keys"
     (let [m {:a 1 :b "b" "a" :a "b" :b}]
@@ -1498,9 +1604,42 @@
                (recur r orig-all (conj pairs pair) (+ accum v1 v2))))))))
 
 (deftest loop-associative-destructuring
+  (testing "ignore non-associative types"
+    (is (= []
+           (loop [items       [nil [] [:a 5] "" "abc" 5 5.5]
+                  {:keys [a]} (first items)
+                  accum       []]
+             (if (seq items)
+               (recur (rest items)
+                      (first (rest items))
+                      (cond-> accum a (conj a)))
+               accum))))
+
+    (testing "with defaults"
+      (is (= (vec (repeat 7 :not-defined))
+             (loop [items                            [nil [] [:a 5] "" "abc" 5 5.5]
+                    {:keys [a] :or {a :not-defined}} (first items)
+                    accum                            []]
+               (if (seq items)
+                 (recur (rest items)
+                        (first (rest items))
+                        (cond-> accum a (conj a)))
+                 accum))))))
+
+  (testing "falsey values not overridden by default values"
+    (is (= [nil false]
+           (loop [items                                  (map #(hash-map :a %) [nil false])
+                  {:keys [a] :as m :or {a :not-defined}} (first items)
+                  accum                                  []]
+             (if (seq items)
+               (recur (rest items)
+                      (first (rest items))
+                      (conj accum a))
+               accum)))))
+
   (testing "destructuring simple keys"
-    (is (= [[{:a "a" :b "b"} {:a "c" :b "d"} {:a "e"}] "abcdee"]
-           (loop [items                           [{:a "a" :b "b"} {:a "c" :b "d"} {:a "e"}]
+    (is (= [[{:a "a" :b "b"} {:a "c" :b "d"} {:a "e"} {:a "f"} {:a "g" :b "h"}] "abcdeefegh"]
+           (loop [items                           [{:a "a" :b "b"} {:a "c" :b "d"} {:a "e"} '(:a "f") '({:a "g" :b "h"})]
                   {:keys [a b] :as m :or {b "e"}} (first items)
                   coll                            []
                   accum                           []]

tests/basilisp/vector_test.py

@@ -124,6 +124,10 @@ def test_val_at():
     assert None is vec.PersistentVector.empty().val_at(0)
     assert None is vec.PersistentVector.empty().val_at(1)
     assert None is vec.PersistentVector.empty().val_at(-1)
+    assert None is vec.PersistentVector.empty().val_at(keyword("blah"))
+    assert "default" == vec.PersistentVector.empty().val_at(keyword("blah"), "default")
+    assert None is vec.PersistentVector.empty().val_at("key")
+    assert "default" == vec.PersistentVector.empty().val_at("key", "default")
 
 
 def test_peek():