Fix nth and get type support (#570)

 * `nth` previously would respond to nearly any collection type, which was incorrect. `nth` should only respond to sequential types, not maps and sets which _can produce_ sequences but are not themselves sequential.
 * `get` should technically respond to all types, though in most cases it returns `nil` (or its default value). It should respond to set types as if by calling them with the key. It should respond to all Python types as by `__getitem__`.

Author: chrisrink10

CHANGELOG.md

@@ -21,6 +21,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
  * Basilisp set and map types are now backed by the HAMT provided by `immutables` (#557)
+ * `get` now responds `nil` (or its default) for any unsupported types (#570)
+ * `nth` now supports only sequential collections (or `nil`) and will throw an exception for any invalid types (#570)
 
 ### Fixed
  * Fixed a bug where the Basilisp AST nodes for return values of `deftype` members could be marked as _statements_ rather than _expressions_, resulting in an incorrect `nil` return (#523)

src/basilisp/core.lpy

@@ -321,9 +321,13 @@
               ~@body)))))
 
 (defn nth
-  "Returns the ith element of coll (0-indexed), if it exists.
-  nil otherwise. If i is out of bounds, throws an IndexError unless
-  notfound is specified."
+  "Returns the `i`th element of `coll` (0-indexed), if it exists or `nil`
+  otherwise. If `i` is out of bounds, throws an `IndexError` unless `notfound`
+  is specified.
+
+  `coll` may be any sequential collection type (such as a list or vector), string,
+  or `nil`. If `coll` is not one of the supported types, a `TypeError` will be
+  thrown."
   ([coll i]
    (basilisp.lang.runtime/nth coll i))
   ([coll i notfound]
@@ -1912,7 +1916,11 @@
   (apply (.-dissoc m) ks))
 
 (defn get
-  "Return the entry of m corresponding to k if it exists or nil/default otherwise."
+  "Return the entry of `m` corresponding to `k` if it exists or `nil`/`default`
+  otherwise.
+
+  `m` may be any associative type (such as a vector or map), set type, or string.
+  If `m` is not one of the supported types, `get` always returns `nil`/`default`."
   ([m k]
    (basilisp.lang.runtime/get m k))
   ([m k default]

src/basilisp/lang/compiler/analyzer.py

@@ -1580,7 +1580,7 @@ def __deftype_and_reify_impls_are_all_abstract(  # pylint: disable=too-many-bran
             if not interface.var.is_bound:
                 logger.log(
                     TRACE,
-                    f"{special_form} interface Var '{interface.form}' is not bound"
+                    f"{special_form} interface Var '{interface.form}' is not bound "
                     "and cannot be checked for abstractness; deferring to runtime",
                 )
                 unverifiably_abstract.add(interface)

src/basilisp/lang/runtime.py

@@ -54,7 +54,6 @@
 from basilisp.lang.reference import ReferenceBase
 from basilisp.lang.typing import CompilerOpts, LispNumber
 from basilisp.lang.util import OBJECT_DUNDER_METHODS, demunge, is_abstract, munge
-from basilisp.logconfig import TRACE
 from basilisp.util import Maybe
 
 logger = logging.getLogger(__name__)
@@ -1032,23 +1031,12 @@ def nth(coll, i: int, notfound=__nth_sentinel):
     """Returns the ith element of coll (0-indexed), if it exists.
     None otherwise. If i is out of bounds, throws an IndexError unless
     notfound is specified."""
-    try:
-        for j, e in enumerate(coll):
-            if i == j:
-                return e
-    except TypeError:
-        pass
-    else:
-        if notfound is not __nth_sentinel:
-            return notfound
-        raise IndexError(f"Index {i} out of bounds")
-
     raise TypeError(f"nth not supported on object of type {type(coll)}")
 
 
 @nth.register(type(None))
 def _nth_none(_: None, i: int, notfound=__nth_sentinel) -> None:
-    return None
+    return notfound if notfound is not __nth_sentinel else None
 
 
 @nth.register(Sequence)
@@ -1059,18 +1047,67 @@ def _nth_sequence(coll: Sequence, i: int, notfound=__nth_sentinel):
         if notfound is not __nth_sentinel:
             return notfound
         raise ex
-    except TypeError as ex:
-        # Log these at TRACE so they don't gum up the DEBUG logs since most
-        # cases where this exception occurs are not bugs.
-        logger.log(TRACE, "Ignored %s: %s", type(ex).__name__, ex)
+
+
+@nth.register(ISeq)
+def _nth_iseq(coll: ISeq, i: int, notfound=__nth_sentinel):
+    for j, e in enumerate(coll):
+        if i == j:
+            return e
+
+    if notfound is not __nth_sentinel:
+        return notfound
+
+    raise IndexError(f"Index {i} out of bounds")
+
+
+@functools.singledispatch
+def contains(coll, k):
+    """Return true if o contains the key k."""
+    return k in coll
+
+
+@contains.register(IAssociative)
+def _contains_iassociative(coll, k):
+    return coll.contains(k)
+
+
+@functools.singledispatch
+def get(m, k, default=None):  # pylint: disable=unused-argument
+    """Return the value of k in m. Return default if k not found in m."""
+    return default
+
+
+@get.register(dict)
+@get.register(list)
+@get.register(str)
+def _get_others(m, k, default=None):
+    try:
+        return m[k]
+    except (KeyError, IndexError):
+        return default
+
+
+@get.register(IPersistentSet)
+@get.register(frozenset)
+@get.register(set)
+def _get_settypes(m, k, default=None):
+    if k in m:
+        return k
+    return default
+
+
+@get.register(ILookup)
+def _get_ilookup(m, k, default=None):
+    return m.val_at(k, default)
 
 
 @functools.singledispatch
 def assoc(m, *kvs):
     """Associate keys to values in associative data structure m. If m is None,
     returns a new Map with key-values kvs."""
     raise TypeError(
-        f"Object of type {type(m)} does not implement Associative interface"
+        f"Object of type {type(m)} does not implement IAssociative interface"
     )
 
 
@@ -1090,7 +1127,7 @@ def update(m, k, f, *args):
     calling f(old_v, *args). If m is None, use an empty map. If k is not in m, old_v will be
     None."""
     raise TypeError(
-        f"Object of type {type(m)} does not implement Associative interface"
+        f"Object of type {type(m)} does not implement IAssociative interface"
     )
 
 
@@ -1112,7 +1149,8 @@ def conj(coll, *xs):
     depending on the type of coll. conj returns the same type as coll. If coll
     is None, return a list with xs conjoined."""
     raise TypeError(
-        f"Object of type {type(coll)} does not implement Collection interface"
+        f"Object of type {type(coll)} does not implement "
+        "IPersistentCollection interface"
     )
 
 
@@ -1228,32 +1266,6 @@ def __ge__(self, other):
     return lseq.sequence(sorted(coll, key=key))
 
 
-@functools.singledispatch
-def contains(coll, k):
-    """Return true if o contains the key k."""
-    return k in coll
-
-
-@contains.register(IAssociative)
-def _contains_iassociative(coll, k):
-    return coll.contains(k)
-
-
-@functools.singledispatch
-def get(m, k, default=None):
-    """Return the value of k in m. Return default if k not found in m."""
-    try:
-        return m[k]
-    except (KeyError, IndexError, TypeError) as e:
-        logger.log(TRACE, "Ignored %s: %s", type(e).__name__, e)
-        return default
-
-
-@get.register(ILookup)
-def _get_ilookup(m, k, default=None):
-    return m.val_at(k, default)
-
-
 def is_special_form(s: sym.Symbol) -> bool:
     """Return True if s names a special form."""
     return s in _SPECIAL_FORMS

tests/basilisp/core_test.py

@@ -1415,10 +1415,8 @@ class TestRandom:
     COLLS = [
         vec.v("a", "b", "c"),
         llist.l("a", "b", "c"),
-        lset.s("a", "b", "c"),
         ["a", "b", "c"],
         ("a", "b", "c"),
-        {"a", "b", "c"},
     ]
 
     @pytest.fixture(scope="class", params=COLLS)

tests/basilisp/runtime_test.py

@@ -156,6 +156,7 @@ def test_apply():
 
 def test_nth():
     assert None is runtime.nth(None, 1)
+    assert "not found" == runtime.nth(None, 4, "not found")
     assert "l" == runtime.nth("hello world", 2)
     assert "l" == runtime.nth(["h", "e", "l", "l", "o"], 2)
     assert "l" == runtime.nth(llist.l("h", "e", "l", "l", "o"), 2)
@@ -164,20 +165,72 @@ def test_nth():
 
     assert "Z" == runtime.nth(llist.l("h", "e", "l", "l", "o"), 7, "Z")
     assert "Z" == runtime.nth(lseq.sequence(["h", "e", "l", "l", "o"]), 7, "Z")
+    assert "Z" == runtime.nth(vec.v("h", "e", "l", "l", "o"), 7, "Z")
 
     with pytest.raises(IndexError):
         runtime.nth(llist.l("h", "e", "l", "l", "o"), 7)
 
     with pytest.raises(IndexError):
         runtime.nth(lseq.sequence(["h", "e", "l", "l", "o"]), 7)
 
+    with pytest.raises(IndexError):
+        runtime.nth(vec.v("h", "e", "l", "l", "o"), 7)
+
+    with pytest.raises(TypeError):
+        runtime.nth(lmap.Map.empty(), 2)
+
+    with pytest.raises(TypeError):
+        runtime.nth(lmap.map({"a": 1, "b": 2, "c": 3}), 2)
+
+    with pytest.raises(TypeError):
+        runtime.nth(lset.Set.empty(), 2)
+
+    with pytest.raises(TypeError):
+        runtime.nth(lset.s(1, 2, 3), 2)
+
     with pytest.raises(TypeError):
         runtime.nth(3, 1)
 
     with pytest.raises(TypeError):
         runtime.nth(3, 1, "Z")
 
 
+def test_get():
+    assert None is runtime.get(None, "a")
+    assert keyword.keyword("nada") is runtime.get(None, "a", keyword.keyword("nada"))
+    assert None is runtime.get(3, "a")
+    assert keyword.keyword("nada") is runtime.get(3, "a", keyword.keyword("nada"))
+    assert 1 == runtime.get(lmap.map({"a": 1}), "a")
+    assert None is runtime.get(lmap.map({"a": 1}), "b")
+    assert 2 == runtime.get(lmap.map({"a": 1}), "b", 2)
+    assert 1 == runtime.get(vec.v(1, 2, 3), 0)
+    assert None is runtime.get(vec.v(1, 2, 3), 3)
+    assert "nada" == runtime.get(vec.v(1, 2, 3), 3, "nada")
+    assert "l" == runtime.get("hello world", 2)
+    assert None is runtime.get("hello world", 50)
+    assert "nada" == runtime.get("hello world", 50, "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")
+    assert 1 == runtime.get({"a": 1}, "a")
+    assert None is runtime.get({"a": 1}, "b")
+    assert 2 == runtime.get({"a": 1}, "b", 2)
+    assert "a" == runtime.get({"a", "b", "c"}, "a")
+    assert None is runtime.get({"a", "b", "c"}, "d")
+    assert 2 == runtime.get({"a", "b", "c"}, "d", 2)
+    assert "a" == runtime.get(frozenset({"a", "b", "c"}), "a")
+    assert None is runtime.get(frozenset({"a", "b", "c"}), "d")
+    assert 2 == runtime.get(frozenset({"a", "b", "c"}), "d", 2)
+    assert "a" == runtime.get(lset.set({"a", "b", "c"}), "a")
+    assert None is runtime.get(lset.set({"a", "b", "c"}), "d")
+    assert 2 == runtime.get(lset.set({"a", "b", "c"}), "d", 2)
+
+    # lists are "supported" by virtue of the fact that `get`-ing them does not fail
+    assert None is runtime.get(llist.l(1, 2, 3), 0)
+    assert None is runtime.get(llist.l(1, 2, 3), 3)
+    assert "nada" == runtime.get(llist.l(1, 2, 3), 0, "nada")
+
+
 def test_assoc():
     assert lmap.Map.empty() == runtime.assoc(None)
     assert lmap.map({"a": 1}) == runtime.assoc(None, "a", 1)