Equality defined between builtin collections respect partitions (#558)

This commit corrects the equality operators between builtin collection types to respect equality partitions. Equality partitions are segments of different collections within which collections can be considered equal and between which collections cannot be considered equal. The three equality partitions are: sequential types (vectors, seqs), maps, and sets.

Several other small changes were made in support of that goal:

 * `Map` types now properly support the full Python `Mapping` protocol.
    * `Map.__iter__` now acts like a standard Python `Mapping` type, returning an iterable of keys. Formerly, it was an iterable of `MapEntry` types, which was occasionally convenient, but definitely un-idiomatic in Python. This change doesn't really affect Basilisp code though, since Basilisp always calls `.seq()` on maps to iterate over entries.
    * Removed the `.iteritems()`, `.iterkeys()`, and `.itervalues()` methods which simply forwarded to the base `pyrsistent.PMap` implementation.
    * `Mapping.items()`, `.keys()`, and `.values()` are now supported. All cases relying on the former `__iter__` implementation have generally been updated to use `.items()` (though a few cases were updated to use `.seq()`).
 * All builtin `ISeq` types and `IPersistentVector` have been annotated with the marker interface `ISequential` which is attached to all sequential types in that equality partition.
 * `IPersistentVector` types are no longer considered `Mapping` types, though they are still considered `IAssociative`. It never really made sense to consider them `Mapping` types and it was generally inconvenient since it is not useful to consider vectors as maps in most cases.
 * Removed the custom `Vector.rseq()` implementation since it was undoubtedly no more efficient than Python's `reversed` using the Python `Sequence` protocol.

Two small fixes unrelated to the main changes were also included because I was already in the code:
 * `basilisp.core/rest` now never returns `nil`. Instead, it returns the empty seq in any case it would formerly return `nil`.
 * I fixed a [deprecation with the PyTest plugin](pytest-dev/pytest#6680) that emitted Warnings every time any Basilisp tests ran.

References:
 * [Clojure Collection Model](https://insideclojure.org/2016/03/16/collections/)
 * [Sequences](https://insideclojure.org/2015/01/02/sequences/)

Author: chrisrink10

src/basilisp/lang/compiler/generator.py

@@ -228,7 +228,7 @@ def __init__(
         self._var_indirection_override: Deque[bool] = collections.deque([])
 
         if logger.isEnabledFor(logging.DEBUG):  # pragma: no cover
-            for k, v in self._opts:
+            for k, v in self._opts.items():
                 logger.debug("Compiler option %s = %s", k, v)
 
     @property

src/basilisp/lang/interfaces.py

@@ -153,9 +153,7 @@ def empty() -> "IPersistentCollection[T]":
 T_assoc = TypeVar("T_assoc", bound="IAssociative")
 
 
-class IAssociative(
-    ILookup[K, V], Mapping[K, V], IPersistentCollection[IMapEntry[K, V]]
-):
+class IAssociative(ILookup[K, V], IPersistentCollection[IMapEntry[K, V]]):
     __slots__ = ()
 
     @abstractmethod
@@ -193,7 +191,7 @@ class IPersistentList(ISequential, IPersistentStack[T]):
 T_map = TypeVar("T_map", bound="IPersistentMap")
 
 
-class IPersistentMap(ICounted, IAssociative[K, V]):
+class IPersistentMap(ICounted, Mapping[K, V], IAssociative[K, V]):
     __slots__ = ()
 
     @abstractmethod
@@ -267,6 +265,23 @@ def _record_lrepr(self, kwargs: Mapping) -> str:
         raise NotImplementedError()
 
 
+def seq_equals(s1, s2) -> bool:
+    """Return True if two sequences contain the exactly the same elements in the
+    same order. Return False if one sequence is shorter than the other."""
+    assert isinstance(s1, (ISeq, ISequential))
+
+    if not isinstance(s2, (ISeq, ISequential)):
+        return NotImplemented
+
+    sentinel = object()
+    for e1, e2 in itertools.zip_longest(s1, s2, fillvalue=sentinel):  # type: ignore[arg-type]
+        if bool(e1 is sentinel) or bool(e2 is sentinel):
+            return False
+        if e1 != e2:
+            return False
+    return True
+
+
 class ISeq(ILispObject, ISeqable[T]):
     __slots__ = ()
 
@@ -320,17 +335,9 @@ def _lrepr(self, **kwargs):
         return seq_lrepr(iter(self), "(", ")", **kwargs)
 
     def __eq__(self, other):
-        sentinel = object()
-        try:
-            for e1, e2 in itertools.zip_longest(self, other, fillvalue=sentinel):
-                if bool(e1 is sentinel) or bool(e2 is sentinel):
-                    return False
-                if e1 != e2:
-                    return False
-        except TypeError:
-            return False
-        else:
+        if self is other:
             return True
+        return seq_equals(self, other)
 
     def __hash__(self):
         return hash(tuple(self))

src/basilisp/lang/keyword.py

@@ -62,13 +62,13 @@ def complete(
         results = filter(
             lambda kw: (kw.ns is not None and kw.ns == prefix)
             and kw.name.startswith(suffix),
-            kw_cache.itervalues(),
+            kw_cache.values(),
         )
     else:
         results = filter(
             lambda kw: kw.name.startswith(text)
             or (kw.ns is not None and kw.ns.startswith(text)),
-            kw_cache.itervalues(),
+            kw_cache.values(),
         )
 
     return map(str, results)

src/basilisp/lang/list.py

@@ -22,9 +22,6 @@ def __init__(self, wrapped: "PList[T]", meta=None) -> None:
         self._inner = wrapped
         self._meta = meta
 
-    def __eq__(self, other):
-        return self._inner == other
-
     def __getitem__(self, item):
         if isinstance(item, slice):
             return List(self._inner[item])

src/basilisp/lang/map.py

@@ -44,7 +44,13 @@ def __contains__(self, item):
         return item in self._inner
 
     def __eq__(self, other):
-        return self._inner == other
+        if self is other:
+            return True
+        if not isinstance(other, Mapping):
+            return NotImplemented
+        if len(self._inner) != len(other):
+            return False
+        return dict(self._inner.iteritems()) == dict(other.items())
 
     def __getitem__(self, item):
         return self._inner[item]
@@ -53,8 +59,7 @@ def __hash__(self):
         return hash(self._inner)
 
     def __iter__(self):
-        for k, v in self._inner.iteritems():
-            yield MapEntry.of(k, v)
+        yield from self._inner.iterkeys()
 
     def __len__(self):
         return len(self._inner)
@@ -64,24 +69,6 @@ def _lrepr(self, **kwargs):
             self._inner.iteritems, start="{", end="}", meta=self._meta, **kwargs
         )
 
-    def items(self):
-        return self._inner.items()
-
-    def keys(self):
-        return self._inner.keys()
-
-    def values(self):
-        return self._inner.values()
-
-    def iteritems(self):
-        return self._inner.iteritems()
-
-    def iterkeys(self):
-        return self._inner.iterkeys()
-
-    def itervalues(self):
-        return self._inner.itervalues()
-
     @property
     def meta(self) -> Optional[IPersistentMap]:
         return self._meta
@@ -140,12 +127,7 @@ def cons(  # type: ignore[override]
         e = self._inner.evolver()
         try:
             for elem in elems:
-                if isinstance(elem, (IPersistentMap, Mapping)) and not isinstance(
-                    elem, IPersistentVector
-                ):
-                    # Vectors are handled in the final else block, since we
-                    # do not want to treat them as Mapping types for this
-                    # particular usage.
+                if isinstance(elem, (IPersistentMap, Mapping)):
                     for k, v in elem.items():
                         e.set(k, v)
                 elif isinstance(elem, IMapEntry):
@@ -169,7 +151,7 @@ def empty() -> "Map":
         return m()
 
     def seq(self) -> ISeq[IMapEntry[K, V]]:
-        return sequence(self)
+        return sequence(MapEntry.of(k, v) for k, v in self._inner.iteritems())
 
 
 def map(kvs: Mapping[K, V], meta=None) -> Map[K, V]:  # pylint:disable=redefined-builtin

src/basilisp/lang/reader.py

@@ -296,11 +296,11 @@ def __init__(  # pylint: disable=too-many-arguments
         process_reader_cond: bool = True,
     ) -> None:
         data_readers = Maybe(data_readers).or_else_get(lmap.Map.empty())
-        for entry in data_readers:
-            if not isinstance(entry.key, symbol.Symbol):
+        for reader_sym in data_readers.keys():
+            if not isinstance(reader_sym, symbol.Symbol):
                 raise TypeError("Expected symbol for data reader tag")
-            if not entry.key.ns:
-                raise ValueError(f"Non-namespaced tags are reserved by the reader")
+            if not reader_sym.ns:
+                raise ValueError("Non-namespaced tags are reserved by the reader")
 
         self._data_readers = ReaderContext._DATA_READERS.update_with(
             lambda l, r: l,  # Do not allow callers to overwrite existing builtin readers

src/basilisp/lang/runtime.py

@@ -639,9 +639,7 @@ def refer_all(self, other_ns: "Namespace") -> None:
         """Refer all the Vars in the other namespace."""
         with self._lock:
             final_refers = self._refers
-            for entry in other_ns.interns:
-                s: sym.Symbol = entry.key
-                var: Var = entry.value
+            for s, var in other_ns.interns.items():
                 if not var.is_private:
                     final_refers = final_refers.assoc(s, var)
             self._refers = final_refers
@@ -716,7 +714,8 @@ def __complete_alias(
         prefix from the list of aliased namespaces. If name_in_ns is given,
         further attempt to refine the list to matching names in that namespace."""
         candidates = filter(
-            Namespace.__completion_matcher(prefix), ((s, n) for s, n in self.aliases)
+            Namespace.__completion_matcher(prefix),
+            ((s, n) for s, n in self.aliases.items()),
         )
         if name_in_ns is not None:
             for _, candidate_ns in candidates:
@@ -739,12 +738,13 @@ def __complete_imports_and_aliases(
         aliases = lmap.map(
             {
                 alias: imports.val_at(import_name)
-                for alias, import_name in self.import_aliases
+                for alias, import_name in self.import_aliases.items()
             }
         )
 
         candidates = filter(
-            Namespace.__completion_matcher(prefix), itertools.chain(aliases, imports)
+            Namespace.__completion_matcher(prefix),
+            itertools.chain(aliases.items(), imports.items()),
         )
         if name_in_module is not None:
             for _, module in candidates:
@@ -771,7 +771,7 @@ def is_match(entry: Tuple[sym.Symbol, Var]) -> bool:
 
         return map(
             lambda entry: f"{entry[0].name}",
-            filter(is_match, ((s, v) for s, v in self.interns)),
+            filter(is_match, ((s, v) for s, v in self.interns.items())),
         )
 
     # pylint: disable=unnecessary-comprehension
@@ -781,7 +781,8 @@ def __complete_refers(self, value: str) -> Iterable[str]:
         return map(
             lambda entry: f"{entry[0].name}",
             filter(
-                Namespace.__completion_matcher(value), ((s, v) for s, v in self.refers)
+                Namespace.__completion_matcher(value),
+                ((s, v) for s, v in self.refers.items()),
             ),
         )
 
@@ -807,13 +808,11 @@ def complete(self, text: str) -> Iterable[str]:
         return results
 
 
-def push_thread_bindings(m: IAssociative[Var, Any]) -> None:
+def push_thread_bindings(m: IPersistentMap[Var, Any]) -> None:
     """Push thread local bindings for the Var keys in m using the values."""
     bindings = set()
 
-    for entry in m:
-        var: Var = entry.key  # type: ignore
-        val = entry.value
+    for var, val in m.items():
         if not var.dynamic:
             raise RuntimeException(
                 "cannot set thread-local bindings for non-dynamic Var"
@@ -853,11 +852,11 @@ def first(o):
     return s.first
 
 
-def rest(o) -> Optional[ISeq]:
+def rest(o) -> ISeq:
     """If o is a ISeq, return the elements after the first in o. If o is None,
     returns an empty seq. Otherwise, coerces o to a seq and returns the rest."""
     if o is None:
-        return None
+        return lseq.EMPTY
     if isinstance(o, ISeq):
         s = o.rest
         if s is None:
@@ -1233,7 +1232,7 @@ def _to_py_map(
 ) -> dict:
     return {
         to_py(key, keyword_fn=keyword_fn): to_py(value, keyword_fn=keyword_fn)
-        for key, value in o
+        for key, value in o.items()
     }
 
 

src/basilisp/lang/seq.py

@@ -1,12 +1,12 @@
 from typing import Any, Callable, Iterable, Iterator, Optional, TypeVar
 
-from basilisp.lang.interfaces import IPersistentMap, ISeq, IWithMeta
+from basilisp.lang.interfaces import IPersistentMap, ISeq, ISequential, IWithMeta
 from basilisp.util import Maybe
 
 T = TypeVar("T")
 
 
-class _EmptySequence(IWithMeta, ISeq[T]):
+class _EmptySequence(IWithMeta, ISequential, ISeq[T]):
     def __repr__(self):
         return "()"
 
@@ -39,7 +39,7 @@ def cons(self, elem: T) -> ISeq[T]:
 EMPTY: ISeq = _EmptySequence()
 
 
-class Cons(ISeq[T], IWithMeta):
+class Cons(ISeq[T], ISequential, IWithMeta):
     __slots__ = ("_first", "_rest", "_meta")
 
     def __init__(
@@ -75,7 +75,7 @@ def with_meta(self, meta: Optional[IPersistentMap]) -> "Cons[T]":
         return Cons(first=self._first, seq=self._rest, meta=meta)
 
 
-class _Sequence(IWithMeta, ISeq[T]):
+class _Sequence(IWithMeta, ISequential, ISeq[T]):
     """Sequences are a thin wrapper over Python Iterable values so they can
     satisfy the Basilisp `ISeq` interface.
 
@@ -128,7 +128,7 @@ def cons(self, elem):
         return Cons(elem, self)
 
 
-class LazySeq(IWithMeta, ISeq[T]):
+class LazySeq(IWithMeta, ISequential, ISeq[T]):
     """LazySeqs are wrappers for delaying sequence computation. Create a LazySeq
     with a function that can either return None or a Seq. If a Seq is returned,
     the LazySeq is a proxy to that Seq."""

src/basilisp/lang/set.py

@@ -1,4 +1,5 @@
-from typing import Iterable, Optional, TypeVar
+from collections.abc import Set as _PySet
+from typing import AbstractSet, Iterable, Optional, TypeVar
 
 from pyrsistent import PSet, pset  # noqa # pylint: disable=unused-import
 
@@ -36,7 +37,11 @@ def __contains__(self, item):
         return item in self._inner
 
     def __eq__(self, other):
-        return self._inner == other
+        if self is other:
+            return True
+        if not isinstance(other, AbstractSet):
+            return NotImplemented
+        return _PySet.__eq__(self, other)
 
     def __ge__(self, other):
         return self._inner >= other

src/basilisp/lang/vector.py

@@ -9,6 +9,7 @@
     IPersistentVector,
     ISeq,
     IWithMeta,
+    seq_equals,
 )
 from basilisp.lang.obj import seq_lrepr as _seq_lrepr
 from basilisp.lang.seq import sequence
@@ -33,12 +34,11 @@ def __contains__(self, item):
         return item in self._inner
 
     def __eq__(self, other):
-        if hasattr(other, "__len__"):
-            return self._inner == other
-        try:
-            return all(e1 == e2 for e1, e2 in zip(self._inner, other))
-        except TypeError:
+        if self is other:
+            return True
+        if hasattr(other, "__len__") and len(self) != len(other):
             return False
+        return seq_equals(self, other)
 
     def __getitem__(self, item):
         if isinstance(item, slice):
@@ -106,15 +106,7 @@ def pop(self) -> "Vector[T]":
         return self[:-1]
 
     def rseq(self) -> ISeq[T]:
-        def _reverse_vec() -> Iterable[T]:
-            l = len(self)
-            for i in range(l - 1, 0, -1):
-                yield self._inner[i]
-
-            if l:
-                yield self._inner[0]
-
-        return sequence(_reverse_vec())
+        return sequence(reversed(self))
 
 
 K = TypeVar("K")